Roderick Colenbrander wrote:
First of all it appears that this patch contains multiple patches (e.g. the direct3d9 getdc p8 check is one). I'll mention some others below. The p8 code is very sensitive to bugs and those are most of the time very hard to fix, so I'm quite strict. Various things need to be tested using testcases before this patch (cut in pieces) can make it in.
All changes are functionally related (direct3d9 format check ensures any palette-related code below it will be ddraw/d3d <= 7 only), but ok, I guess it's possible to cut it into several pieces.
In case of read_from_framebuffer when there is no palette set, just return directly instead of adding an if(pal) check. The function won't do anything useful without a palette. Further read_from_framebuffer shouldn't get called from d3d8/9 games anyway as there aren't p8 render targets.
Agree about this.
I wonder if we need SetDibColorTable at all in case of GetDC as the data the app will receive in the hdc is 8bit. It wants to draw in it by hand, so I would say that the palette won't matter. It might require a testcase.
I have no idea if any app actually uses it, but, hypothetically, it can call GetDIBColorTable, which seems to return meaningful results on native.
The removal of the device palette code from RealizePalette can be done in a separate patch.
I'm not sure if change in SetColorKey is correct in the case a surface doesn't have a palette and uses the primary surface its palette. It doesn't feel right. I think it should fail but this requires a testcase.
Second even if the call to SetDibColorTable is needed I wonder what should happen on surfaces which don't have a palette set. It might be correct to use the palette of the primary surface but it might also just be an illegal situation.
I assume you mean GetDC because I didn't change SetColorKey. I did some tests in windows XP, if you obtain a dc from offscreen surface without a palette, then call GetDIBColorTable on this dc, you get a color table that matches palette of primary surface. I just replicated the existing functionality in Wine which was doing the same only using device palette as intermediary storage of primary palette: when setting palette to primary surface, entries were copied to current device palette, but when palette is missing in GetDC it was using current device palette that's the same as primary surface palette. I guess it's most likely obscure and unused functionality, but who knows if there's something that may rely on it.
I can integrate a test for this scenario (GetDC/GetDIBColorTable with offscreen surface that has not palette and compare it to primary surface palette) into ddraw testcases, if needed. Do you have ideas what else can be tested?