Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCMemcard: Read comments, banners, and icons via logical data offsets instead of physical ones. #8302

Open
wants to merge 3 commits into
base: master
from

Conversation

@AdmiralCurtiss
Copy link
Contributor

commented Aug 7, 2019

Previously the code assumed that the data for the comments, banners, and icons was contiguous from the first block of the data's physical offset in the memory card, rather than referring to the logical position within the referenced save file, which is not only incorrect for the format but could also lead to out-of-bounds memory accesses. This should now be rectified.

Fixes:
https://bugs.dolphin-emu.org/issues/10600
https://bugs.dolphin-emu.org/issues/11820

Please scrutinize this, I'm not entirely sure if this is all correct. It seems to give correct results for all memory cards I have access to, but considering how long this old approach seemingly worked who knows.

The Icon/Banner code could probably use some additional rewriting on top of this to be honest, it's a mess. It also seems to disagree with itself on how the bnr_format should actually be interpreted...

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-comments branch 2 times, most recently from 9c7ccad to cce5689 Aug 7, 2019

std::array<u8, 8> frame_delays;
std::array<u32, 8> frame_offsets;
bool has_shared_palette = false;
for (int i = 0; i < 8; ++i)

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Aug 7, 2019

Member

Something tells me the 8 here (and above in the std::array declarations) should be a constant...not immediately sure where it comes from though.

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Aug 7, 2019

Author Contributor

It's the maximum amount of frames in an icon animation, which results from the 16 bit values storing the per-frame format and delay at 2 bits each. Should probably be a constant, yeah.


switch (bnrFormat)
switch (bnr_format)

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Aug 7, 2019

Member

I wonder if we should at least document in some way that the values 0 (which is "no banner" iirc?) and 3 (which should be CI8 with unique palette?) don't affect the image_offset...altho, since both 1 and 3 use a palette it feels like both might need some offsetting? Is this actually correct?

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Aug 7, 2019

Author Contributor

Yeah this is sketchy, I agree. Is there any banner that uses the '3' format? Maybe we should craft our own and see how the GC BIOS interprets it...

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Aug 8, 2019

Author Contributor

Okay so I played around with this in the GC BIOS and as far as I can tell:

  • The first two bits of the value at 0x7 (m_banner_and_icon_flags) are used for the banner format.
  • '0' means no banner.
  • '1' means paletted (8 bit per pixel + 16 bit color palette)
  • '2' means direct color (16 bit per pixel)
  • '3' acts the same as '0', no banner.

So this code in ReadAnimRGBA8() for the offset is actually correct, strange as it seems. The code in ReadBannerRGBA8() is wrong, as it produces wrong results in the '3' case -- except in the case of 0xFB, which happens to give the correct result of 'no banner'.

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Aug 8, 2019

Author Contributor

Okay, I updated the code with my findings and left some comments. Does this look understandable to you now?

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Aug 8, 2019

Member

Seems fine! Maybe we could peek into the GC BIOS to gain more insight, but this is a good start for now.

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Aug 9, 2019

Author Contributor

GC BIOS seems to agree -- in the PAL BIOS, look at 0x8131cf48 to see it mask off the last two bits and then branch depending on the four different possibilities of the value, with the branches for 0 and 3 both going to 0x8131D044.

const int pixels = 96 * 32;
const u32 pixel_count = 96 * 32;
const u32 palette_indices = 256;
const bool is_ci8 = (bnr_format & 1) != 0;

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Aug 7, 2019

Member

I wonder if this is really 4 possible values (0..3) and not just two bit flags (first one for 8/16 bit, second one for direct/indexed color; with both zero indicating a special "none" value).
YAGCD documents the former, but how we use it here practically means the values 1 and 3 aren't any different (which also ties into the comment down below for the animation).

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-comments branch 3 times, most recently from 2d6367d to 435b10c Aug 8, 2019

AdmiralCurtiss added some commits Aug 6, 2019

GCMemcard: Read banners according to logical data offsets instead of …
…physical data offsets. Also gets rid of some undefined behavior.
GCMemcard: Read icons according to logical data offsets instead of ph…
…ysical data offsets. Also gets rid of some undefined behavior.

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-comments branch from 435b10c to 5efd722 Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.