HackRVA / badge2024

HackRVA 2024 badge firmware and emulator
5 stars 0 forks source link

Display goes white coming back from screensavers #5

Open smcameron opened 5 months ago

smcameron commented 5 months ago

There might be additional issues.

See this old issue from last year: https://github.com/HackRVA/badge2023/issues/10

smcameron commented 5 months ago

Not sure this is related, but I noticed a design oddity about how we handle images.

  1. We go through this laborious image pre-processing step, where we make a list of colors used by the image and store them as RGB values in a "cmap[]" (color map) at 8 bits per R, G and B (24-bits per color). Then we have a list of color index values (pixdata[]) which index into the cmap[] array. These can be 1, 2, 4, or 8 bits per pixel, so you can have 8, 4, 2, or 1 pixel per index value. That's all well and good.
  2. When drawing images, we go through the pixdata, pull out the indexed R, G, and B values from cmap[], and then do the following transformation (pixel is a uint16_t):
                    pixel = ((((r >> 3) & 0b11111) << 11 )
                         |  (((g >> 3) & 0b11111) <<  6 )
                         |  (((b >> 3) & 0b11111)       )) ;

Question: Is there a good reason not to do this transformation in the pre-processing step, so that this transformation is already stored in cmap[]?

The pre-processed data is already uneditable, and if we need to change the images, we re-pre-process them. So seems like this final transformation should also be a part of the pre-processing step, and has the added benefit of reducing the size of cmap by a factor of 0.66 without any loss in display quality.

There is no "alpha-channel" support. There is limited transparency support, as one color index in cmap is designated "transparent" and simply not drawn. The details of how or what this color is are not yet known to me.

All this is leading up to (you guessed it) I am contemplating re-writing the image pre-processing stuff (in C, natch) -- would only support png -- and modifying the image painting functions to deal with the more-highly-preprocessed cmap[] values.

There are potentially stored images in the repo in the old format for which we no longer have the source images though, so may have to write some conversion program to deal with those.

Am I missing something?

paulbruggeman commented 5 months ago

This was done in the original PIC32 based code to save space in the flash (128k) The packing was similar (same?) as the LCD 16 bit format.

-Paul

On Thu, Apr 11, 2024 06:52:01AM -0700, smcameron wrote:

Not sure this is related, but I noticed a design oddity about how we handle images.

  1. We go through this laborious image pre-processing step, where we make a list of colors used by the image and store them as RGB values in a "cmap[]" (color map) at 8 bits per R, G and B (24-bits per color). Then we have a list of color index values (pixdata[]) which index into the cmap[] array. These can be 1, 2, 4, or 8 bits per pixel, so you can have 8, 4, 2, or 1 pixel per index value. That's all well and good.
  2. When drawing images, we go through the pixdata, pull out the indexed R, G, and B values from cmap[], and then do the following transformation (pixel is a uint16_t):
                   pixel = ((((r >> 3) & 0b11111) << 11 )
                        |  (((g >> 3) & 0b11111) <<  6 )
                        |  (((b >> 3) & 0b11111)       )) ;

Question: Is there a good reason not to do this transformation in the pre-processing step, so that this transformation is already stored in cmap[]?

The pre-processed data is already uneditable, and if we need to change the images, we re-pre-process them. So seems like this final transformation should also be a part of the pre-processing step, and has the added benefit of reducing the size of cmap by a factor of 0.66 without any loss in display quality.

There is no "alpha-channel" support. There is limited transparency support, as one color index in cmap is designated "transparent" and simply not drawn. The details of how or what this color is are not yet known to me.

All this is leading up to (you guessed it) I am contemplating re-writing the image pre-processing stuff (in C, natch) -- would only support png -- and modifying the image painting functions to deal with the more-highly-preprocessed cmap[] values.

There are potentially stored images in the repo in the old format for which we no longer have the source images though, so may have to write some conversion program to deal with those.

Am I missing something?

-- Reply to this email directly or view it on GitHub: https://github.com/HackRVA/badge2024/issues/5#issuecomment-2049743315 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

smcameron commented 5 months ago

So I am correct to presume that there is no reason not to do the final step:

                    pixel = ((((r >> 3) & 0b11111) << 11 )
                         |  (((g >> 3) & 0b11111) <<  6 )
                         |  (((b >> 3) & 0b11111)       )) ;

as part of the pre-processing?

paulbruggeman commented 5 months ago

I don't see a problem if there is room in the flash. I'm not sure if the LCD being used now has the same color/bit packing.

Paul

On Thu, Apr 11, 2024 07:51:40AM -0700, smcameron wrote:

So I am correct to presume that there is no reason not to do the final step:

                   pixel = ((((r >> 3) & 0b11111) << 11 )
                        |  (((g >> 3) & 0b11111) <<  6 )
                        |  (((b >> 3) & 0b11111)       )) ;

as part of the pre-processing?

-- Reply to this email directly or view it on GitHub: https://github.com/HackRVA/badge2024/issues/5#issuecomment-2049876758 You are receiving this because you commented.

Message ID: @.***>

smcameron commented 5 months ago

Actually it will take less room in the flash. Currently, in cmap[], we store 24 bits per color. and in pixdata, 1, 2, 4, or 8 bits per pixel. With the new scheme we'd store 16 bits per color in cmap[], and the same 1, 2, 4, or 8 bits per pixel in pixdata. At least if I understand things correctly.

paulbruggeman commented 5 months ago

cmap has 0-256 RGB entries based on packing type of the pix data.

So a "mono" 2 bit image would have 2 RGB entries one for 0 and another for 1 and the pix data would pack 8 pixels per byte.

A 256 color image would have all 256xRGB colors

At least that is what is originally was.

-Paul

On Thu, Apr 11, 2024 08:26:01AM -0700, smcameron wrote:

Actually it will take less room in the flash. Currently, in cmap[], we store 24 bits per color. and in pixdata, 1, 2, 4, or 8 bits per pixel. With the new scheme we'd store 16 bits per color in cmap[], and the same 1, 2, 4, or 8 bits per pixel in pixdata. At least if I understand things correctly.

-- Reply to this email directly or view it on GitHub: https://github.com/HackRVA/badge2024/issues/5#issuecomment-2049958462 You are receiving this because you commented.

Message ID: @.***>

smcameron commented 5 months ago

Right, and I'm not suggesting changing how pixdata works. The only thing I'm suggesting to change is the format of cmap[], which currently has RGB colors with 8 bits per R, G, and B (24 bits per color), which we currently crunch down to 16 bits per color on the fly whenever we draw them. I just suggesting to do this crunching down to 16 bits before packing into cmap[], saving space and time.

smcameron commented 5 months ago

Another data point. Was running my image test app...

Before the screensaver: before-saver

After the screensaver: after-saver