CD-i: Add RGB Decoding #13246
CD-i: Add RGB Decoding #13246
Conversation
This fix adds RGB decoding and RGB Transparency bit. It disables transparency color keying for RGB. It fixes an incorrect enum that was foreshadowed in previous revisions.
src/mame/philips/mcd212.cpp
Outdated
| const bool transp_always = (transp_ctrl_masked == TCR_COND_1); | ||
| const bool invert_transp_condition = BIT(transp_ctrl, 3); | ||
| const int region_flag_index = 1 - (transp_ctrl_masked & 1); | ||
| const bool *region_flags = m_region_flag[region_flag_index]; | ||
| const bool use_region_flag = (transp_ctrl_masked >= TCR_COND_RF0_1 && transp_ctrl_masked <= TCR_COND_RF1KEY_1); | ||
| bool use_color_key = (transp_ctrl_masked == TCR_COND_KEY_1 || transp_ctrl_masked == TCR_COND_RF0KEY_1 || transp_ctrl_masked == TCR_COND_RF1KEY_1); | ||
| bool use_color_key = (icm != ICM_DYUV && !(icm == ICM_RGB555 && Path == 1)) && (transp_ctrl_masked == TCR_COND_KEY_1 || transp_ctrl_masked == TCR_COND_RF0KEY_1 || transp_ctrl_masked == TCR_COND_RF1KEY_1); |
There was a problem hiding this comment.
This condition is starting to grow up excessively out of the sake of respecting enums a bit too much (defeating the purpose of readability).
It also looks something that have performance implications, it looks to me that a Color Key check is done with transp_ctrl_masked bit 0 then bit 3 reverses the meaning?
At bare minimum this needs a // FIXME: because latter doesn't seem implemented anyway.
There was a problem hiding this comment.
The reference enum table from the greenbook spec is added to a comment below. Color Key is used only when the control type is 1, 5 or 6 (or 9, 13, or 14 when negated). I'm having trouble coming up with a good way to formulate it. The enum check is probably the clearest.
Hopefully with the variable renaming and logic patch, it's a little bit easier to understand the logic flow.
The FIXME is probably not needed. The original code does reverse it, but it used logic that was indeed both hard to understand, and incorrect for comparison type 13 and 14 (see notes in the patch). As far as I know, the patched version no longer has any known logic issues, unless someone has a reference from a real console.
I gave a cursory glance yesterday, |
1. Changes the way that negated transparency condition is calculated. 2. Renames COND enums to be easier to read. 3. Renames `transp` variables to be more succinct. 4. Allman Style
|
Sadly there doesn't seem to be more trivial bit logic, as the enums the core uses are not lined up in a convenient way. Instead, the enums and variables were renamed for readability. Additionally, I'm including a change to the way that negated transparency is calculated. This was something the spec was not very clear on, however I think it's likely correct now. There are no known tests that check the changed condition, however all tests looks the same so it probably didn't make things worse.
This matches the greenbook definition now. I don't know of any real cases that use this condition. All tests that I checked looked the same as far as I could tell. |
|
@MooglyGuy could you give this a quick look? |
As with previous pulls, you can validate this using the Validation Disc (Europe) for the CD-i.
RGB Scroll Test
Due to the enum for CLUT8 and RGB being the same, this previously decoded as a CLUT8 image. When ICM flag 1 is used on path 1, it is instead decoded as RGB.
RGB Draw Test
The house is now the right color
Color Keying Test
RGB is now decoded correctly. Additionally, RGB does not match color key. Even when matching "red" colors, notice that the line of RGB still shows red. This is correct behavior, as RGB is not allowed to match color keys.
Transparency Control (Uncertain Issue)
The implementation of RGB color keying is to always be false. However the spec says that "RGB cannot use color key tests". Since this would be a misconfiguration in practice... I am not sure what a true console would produce. The Transparency Control shows a different behavior. It's not clear from looking at it whether the change is expected or unexpected.
Despite the last image, this change is much closer to correct spec behavior and it's recommended to pull. Someone with a real console should at some point run through all visual tests to get proper reference images.