full_screen_refresh vs palette_control

reyalp

14118

« on: 03 / May / 2021, 21:27:16 »

Advertisements

While looking at the issues with ixus240, I noticed that on most(?) pre-digic 6 dryos platforms, full_screen_refresh and palette_control are the same variable (the former is treated as a word and the latter as byte

), found by the sig finder using different matches.

I don't know that this is worth cleaning up, but most ports only use palette control in load_chdk_colors, where it's used like

Code: [Select]

void load_chdk_palette() {
...
    palette_control = 1;
    vid_bitmap_refresh();
...

void vid_bitmap_refresh() {
...
    full_screen_refresh |= 3;
...

suggesting it could be eliminated completely.

thumb2 firmware use palette_control for clean overlays, and do not have full_screen_refresh

Logged

Don't forget what the H stands for.

Caefix

947
Sorry, busy deleting test shots...

Re: full_screen_refresh vs palette_control

« Reply #1 on: 04 / May / 2021, 11:14:00 »

An edited palette collection:

Code: [Select]

G:\MISC\platform\ixus130_sd1400
G:\MISC\platform\ixus200_sd980
G:\MISC\platform\ixus220_elph300hs
G:\MISC\platform\ixus230_elph310hs
G:\MISC\platform\ixus240_elph320hs
G:\MISC\platform\ixus310_elph500hs
G:\MISC\platform\ixus870_sd880
G:\MISC\platform\ixus970_sd890
G:\MISC\platform\ixus990_sd970
G:\MISC\platform\s90
G:\MISC\platform\s95
G:\MISC\platform\sx210

SomeEditedPalettes.zip (9.06 kB - downloaded 9 times.)

Logged

All lifetime is a loan from eternity.

reyalp

14118

Re: full_screen_refresh vs palette_control

« Reply #2 on: 05 / May / 2021, 01:13:44 »

A related, but more important issue with load_chdk_colors, from https://chdk.setepontos.com/index.php?topic=9005.msg145890#msg145890

Most ports do not check if vid_get_bitmap_active_palette returned NULL in load_chdk_colors. Additionally, most ports add a small offset to the value obtained from palette_buffer[active_palette_buffer] or equivalent with palette_buffer_ptr (or hard coded addresses), so NULL isn't returned even if the underlying value was NULL.

The result is load_chdk_colors will clobber memory around CHDK_COLOR_BASE*4, which ends up in ITCM near stuff related to exception handlers.

It also appears the initial value of active_palette_buffer is 0xff, which would likely to give incorrect results if used.

IMO, vid_get_bitmap_active_palette and load_chdk_palette should be fixed so custom colors aren't written if the palette is null. This will unfortunately involve touching a lot of ports.

Logged

Don't forget what the H stands for.

philmoz

3450

Re: full_screen_refresh vs palette_control

« Reply #3 on: 05 / May / 2021, 02:55:28 »

I think this is only a significant problem for cameras that have a 'Color Options' setting in the Canon Menu and the option has been changed from the default value. I was not aware of this until you pointed out the option in another thread.

Cameras with this option will probably have a value of 16 for the playback active_palette_buffer; but apart from the IXUS 310 none of the other ports use this. So identifying affected cameras is also an issue. I have no idea how many cameras offer this option.

The fix for affected cameras is to add the value of param 0x1a to the active_palette_buffer value to get the correct palette_buffer array index.

The initial value of active_palette_buffer should only affect live view or the palette check module and neither of these are likely to be active on startup of the camera.

Some more error checking would not hurt.

Logged

CHDK ports:
sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
g12 (1.00c, 1.00e, 1.00f & 1.00g)
sx130is (1.01d & 1.01f)
ixus310hs (1.00a & 1.01a)
sx40hs (1.00d, 1.00g & 1.00i)
g1x (1.00e, 1.00f & 1.00g)
g5x (1.00c, 1.01a, 1.01b)
g7x2 (1.01a, 1.01b, 1.10b)

philmoz

3450

Re: full_screen_refresh vs palette_control

« Reply #4 on: 05 / May / 2021, 03:11:06 »

Quote from: reyalp on 03 / May / 2021, 21:27:16

I don't know that this is worth cleaning up, but most ports only use palette control in load_chdk_colors, ...

As far as I can tell it makes no difference to the code having both in stubs_entry.S.

Because they are DEF values they don't use any space in the data section - only the actual value gets used in the code.

Phil.

Logged

CHDK ports:
sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
g12 (1.00c, 1.00e, 1.00f & 1.00g)
sx130is (1.01d & 1.01f)
ixus310hs (1.00a & 1.01a)
sx40hs (1.00d, 1.00g & 1.00i)
g1x (1.00e, 1.00f & 1.00g)
g5x (1.00c, 1.01a, 1.01b)
g7x2 (1.01a, 1.01b, 1.10b)

reyalp

14118

Re: full_screen_refresh vs palette_control

« Reply #5 on: 05 / May / 2021, 12:54:55 »

Quote from: philmoz on 05 / May / 2021, 02:55:28

I think this is only a significant problem for cameras that have a 'Color Options' setting in the Canon Menu and the option has been changed from the default value. I was not aware of this until you pointed out the option in another thread.

FWIW, I encountered other cases related to TV out where it could be NULL on elph130 (https://chdk.setepontos.com/index.php?topic=13451.msg142423#msg142423), which doesn't have the color option.

Quote

Cameras with this option will probably have a value of 16 for the playback active_palette_buffer; but apart from the IXUS 310 none of the other ports use this. So identifying affected cameras is also an issue. I have no idea how many cameras offer this option.

Yeah, I just came to the same conclusion in that thread. Copying here:

Quote from: reyalp on 05 / May / 2021, 12:49:17

The normal vid_get_bitmap_active_palette logic is only valid for the default color scheme (orange). When a different color scheme is used, the location in palette_buffer is offset by the number of the color scheme. This logic appears in ixus240 102a FUN_ff2ddd74 (ixus140 equivalent is FUN_ff20f904)

That's why in post 295, caefix's rmem -i32 0x194b88 shows palette_buffer[0] being NULL, and palette_buffer[1] having a value, while koshy's (set to orange, has a a value in palette_buffer[0].

This also explains why playback is 16 rather than 4, because there needs to be space for each color option. This may provide a way of detecting the affected firmwares.

We could use get_parameter_data(0x1a,1) in vid_get_bitmap_active_palette replicate this, but it's not clear the current CHDK custom entries will be appropriate in different Canon palettes. I'm also not sure we want to call get_parameter_data so often.

It seems like there should be a variable somewhere that has the offset already applied or points to the actual current palette, but I haven't seen one.

Making vid_get_bitmap_active_palette and load_custom_colors handle NULLs correctly will at least avoid the memory corruption, and people affected may complain that the colors are messed up.

All cameras that offer this color option will need to a fix. I'm not sure how many there are, none of my cams have it, but the firmware code still checks the flash param.

Logged

Don't forget what the H stands for.

philmoz

3450

Re: full_screen_refresh vs palette_control

« Reply #6 on: 05 / May / 2021, 17:54:58 »

Quote from: reyalp on 05 / May / 2021, 12:54:55

We could use get_parameter_data(0x1a,1) in vid_get_bitmap_active_palette replicate this, but it's not clear the current CHDK custom entries will be appropriate in different Canon palettes.

On the IXUS 310 the alternate palettes have the same layout with just the Canon UI tint colours adjusted - makes sense as otherwise all the icons and code would have to change pixel values instead of just changing the palette entries.

Phil.

Logged

CHDK ports:
sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
g12 (1.00c, 1.00e, 1.00f & 1.00g)
sx130is (1.01d & 1.01f)
ixus310hs (1.00a & 1.01a)
sx40hs (1.00d, 1.00g & 1.00i)
g1x (1.00e, 1.00f & 1.00g)
g5x (1.00c, 1.01a, 1.01b)
g7x2 (1.01a, 1.01b, 1.10b)

philmoz

3450

Re: full_screen_refresh vs palette_control

« Reply #7 on: 05 / May / 2021, 18:54:24 »

Patch attached to ensure vid_get_bitmap_active_palette returns 0 for NULL palette and check for valid palette in load_chdk_palette.

@reyalp - can you check that I got all the offsets correct in vid_get_bitmap_palette please.

Phil.

palette_checks.patch (28.81 kB - downloaded 6 times.)

Logged

CHDK ports:
sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
g12 (1.00c, 1.00e, 1.00f & 1.00g)
sx130is (1.01d & 1.01f)
ixus310hs (1.00a & 1.01a)
sx40hs (1.00d, 1.00g & 1.00i)
g1x (1.00e, 1.00f & 1.00g)
g5x (1.00c, 1.01a, 1.01b)
g7x2 (1.01a, 1.01b, 1.10b)

reyalp

14118

Re: full_screen_refresh vs palette_control

« Reply #8 on: 05 / May / 2021, 19:48:26 »

Quote from: philmoz on 05 / May / 2021, 18:54:24

Patch attached to ensure vid_get_bitmap_active_palette returns 0 for NULL palette and check for valid palette in load_chdk_palette.

Nice. Thanks very much for doing this

Quote

@reyalp - can you check that I got all the offsets correct in vid_get_bitmap_palette please.

The only one that seems off is sx230, original code was +8, patch is +4

On A3400, load_chdk_palette checks for > 0x1900, which is safe but not longer required. It also uses active_palette_buffer & 7, which would not be right on cams with the color option. From the manual, it doesn't look like A3400 has the option, and I didn't see the & on any other cams using active_palette_buffer

Logged

Don't forget what the H stands for.

philmoz

3450

Re: full_screen_refresh vs palette_control

« Reply #9 on: 05 / May / 2021, 21:59:33 »

Thanks for checking - I triple checked it; but still missed the sx230

Updated version - also aligned the a3400 to match the rest.

If this is ok I will check it in to both trunk and 1.5.

palette_checks_v2.patch (29.3 kB - downloaded 7 times.)

Logged

CHDK ports:
sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
g12 (1.00c, 1.00e, 1.00f & 1.00g)
sx130is (1.01d & 1.01f)
ixus310hs (1.00a & 1.01a)
sx40hs (1.00d, 1.00g & 1.00i)
g1x (1.00e, 1.00f & 1.00g)
g5x (1.00c, 1.01a, 1.01b)
g7x2 (1.01a, 1.01b, 1.10b)