replacing "new array[]" by static array causes corrupted image portion

[stevstrong]

Hi Bodmer,

first of all, thank you for this fantastic lib.

I am using a 480x320 ILI9486 TFT display with a "blue pill", and I had the problem of largely increased flash and RAM usage because of the "new uint16_t[]" constructor in line 335 of the JPEG decoder. As a solution, I replaced the mentioned line by declaring a static array uint16_t img[16*16];, and I assigned the pImage variable to this array. Here are the changes.

Now, using the TFT_flash_jpg_v2A.ino sketch, I have the problem that the last MCU block (right bottom 16x16 pixels) of each decoded image is corrupted, and funnily, it shows the same corrupted pixel block for each image. I tried to increase the newly introduced array size, but it didn't help.

Could you please check what am I doing wrong? Or is there maybe a small bug somewhere in the software?

[Bodmer]

You have 2 problems and I am not sure how they are linked in your setup. The "new uint16_t[]" should be creating an array of variable size with a maximum of 16x16 and thus be a maximum of 512 bytes.

I would not expect that hard coding the numbers would save a significant amount of RAM. By hard coding the 16x16 you have introduced a bug, you will get image corruption unless your image is an integral multiple of 16 pixels wide and high, this is because images are always coded in 16x16 blocks, so a 17x17 image would be coded as a 32x32 image but when decoded the mcu size will be adjusted to 16x16 for the first block, 1x16 for the second, 16x1 for the third and 1x1 for the fourth (bottom right block). As you have fixed the block size the array pixels not filled will contain garbage, this garbage will consist of discarded variable values hence the fixed pattern.

It sounds like the solution you need is to use a board with more RAM.

[stevstrong]

Bodmer, thanks for the answer.

It is true, I have two problems. But at the moment I am only concerned about the second one (corrupted display portion), trying to understand it.

You say, I introduced a bug. It is possible. But... Following your explanation, it seems that the display part shall display pixel data dependent on the size of this array, which is dependent on the MCU size, which can vary. If this is correct, can you please point out which place in your library will perform the wrong pattern displaying? Because as far as I can see, no place in your library nor in any example sketch is checking the size of the created pImage array.

The display related routines in the example sketch are using only a pointer to this pImage array, and use other variables (MCUWidth, MCUHeight, MCUx, MCUy and so on) to determine the MCU size and coordinates. So, in my opinion, if this array is large enough to handle the worse case situation (16x16 pixels a 16 bits MCU), and if I don't touch those other variables (and I do not change them, as one can see from the changes linked above), everything should work fine. Meaning, whether it is 16x16, 16x1, 1x16 or 1x1 MCU, all the pixel data will fit in this array in every situation, and the other variables should assure a correct display of each MCU.

Please tell me where exactly is my logic wrong.

EDIT

• Btw, all images are those from your repo, 480x320, being multiple of 16x16.
• All MCUs at the end of each row and each column are all OK, except that in the very last row and last column (bottom right).
• Is this issue really solved, that's why you closed it?

[Bodmer]

I'll see if I can reproduce the problem but I don't have an ST32 to try at the moment. It could be that the library is fine but the sketch is wrong.

[stevstrong]

I am using the sketch from your repo (see my first post), only adapted it to my display type and the corresponding lib. You could try to implement the changed version (see my first post) on Mega or Due, I think it will show the same effect. And if not, then this issue is obviously related only to my setup, in which case I will try to debug it.

[Bodmer]

OK, I've tried it on a Mega and Due the images are fine on a 480x320 HX8257C display with hard coded 16x16 array. As a Mega only has 8K of RAM this would suggest that RAM is not a problem for the sketch.

Also tried it on a ESP8266 (with some sketch tweaks for a different display driver) and it is fine.

It looks like the problem is elsewhere. Try to get back some RAM by weeding out some unimportant functions and see if that is the problem. Other than that see if you can change the fault signature by adding in more code (or create an array in RAM for example) that makes the problem worse, this may point you towards shortage of RAM. Sorry I cannot help more without reproducing your setup.

[stevstrong]

OK, it seems that I have found the root cause.

The line in question is in the function decode_mcu. If I remove that line which is deleting the pImagearray, then the last MCU block is displayed correctly. It seems that the pImage is deleted too soon, before the last block is going to be displayed. Here is a short snippet from my debug log output, when monitoring the status of line 86 of the same function:

drawing eagle_eye...
>>>>>>>>>>>>>>>>>>>>>>>>>
gMaxMCUXSize: 16
gMaxMCUYSize: 16
gNumMCUSRemaining: 361
===============
JPEG image info
===============
Width      :300
Height     :300
Components :1
MCU / row  :19
MCU / col  :19
Scan type  :4
MCU width  :16
MCU height :16
===============
...
mcu nr: 356
mcu nr: 357
mcu nr: 358
mcu nr: 359
status: 1
mcu nr: 360
null pointer at mcu nr: 360, mcu_x: 297, mcu_y: 377
 JPEG drawn in 556 ms 
@@@@@@@@@@@@@@@@@@@@@@@@@

The funny same pattern was due to data found at null pointer, which is always the same :) If I remove that line, everything works fine. Could you please check and confirm my observation?

[Bodmer]

OK, that is interesting, thanks for investigating this!

Unfortunately if that delete is removed then I suspect you will have a "memory leak" because the "new" directive puts the array on the stack and if delete is not called then it is left there and the stack creeps down with time and after drawing lots of jpegs the processor crashes.

I guess the different processor compilers are handling the problem in different ways and on the processors I have used the last MCU block is still on the stack and valid.

I will think on this further, in the meantime I suggest you call abort() function after drawing the jpeg to ensure the array is deleted from the stack.

[Bodmer]

OK, I can see what is happening. I think what we need to do is delete the array from the stack in this function:

int JPEGDecoder::read(void)

So change the is_available check to:

if(is_available == 0) {
    delete pImage;
    return 0;
}

[Bodmer]

OK, I can now verify that the "delete pImage" was in the wrong place and should be removed from the decode_mcu() function and placed in the read() function as noted above.

I managed to reproduce your problem on a ESP8266 by creating a totally black jpeg encoded image. I then see a single faint green spurious pixel that shows up in top left corner of the last MCU block. Making the above change fixes this.

Well done! Thanks for finding this issue, I will update the repository.

[stevstrong]

I can confirm that your new version solves the corrupted block problem. Thank you for correcting this. Just one more observation: during merging the new version from your repo I compared the new version with the old version and it seems that a previous line, which could be important for grayscale images, is missing from the new version: pDst_block += row_pitch; Is this deliberately left away? As I did not test it for grayscale images, I cannot say whether this line should be included or not. Please check this.

[Bodmer]

Thanks again for your dilligence, that has been fixed, there was another missing line and that is fixed as well!

[Bodmer]

I have written a new rendering function and included it in the examples, so now arbitrary sized images can be drawn without extra repeated pixels on right and bottom edges.

[stevstrong]

Wow, really nice, can you please point out which exactly is the mentioned function?

One more question: what exactly is the difference between "Bodmer compatible" and "Adafruit compatible" examples, beside the used TFT library? I couldn't figure out.

[Bodmer]

This is it

Note that I have added a JpegDec.readSwappedBytes() call but you might need the alternative JpegDec.read() depending on the byte order needed for your setup.

[Bodmer]

If you use read() then the pointer setup is different so then use this render function.

[Bodmer]

The menu is a bit confusing, and needs more thought.

Bodmer compatible means you can use the libraries from my repository and a NodeMCU. Adafruit compatible is for the Adafruit GFX and ILI9341 libraries and the Adafruit Huzzah board

[stevstrong]

Thank you. I can now see, you have restructured the examples. If I may make an observation: there is a lot of overlapping code between the different example directories. That's why I would not make different directories for different libraries, neither for different controllers. Reasoning: you have already make clear in the readme that your lib requires those two critical display related functions from the user's TFT library. So once those are used, the rest is actually same for all controllers: any of them may support at least one of use SPIFFS, use SDfat or use arrays from flash. After all, you have now the collection of used functions in JPEG_functions.ino, so the user can choose the functions best suited for its application. You could then eventually explain in the readme which function from the collection would best suited for a special application/controller combination.

[Bodmer]

Unfortunately it has taken longer than expected to get various associated hobby projects off the ground and so the documentation that will explain these things is missing and I wanted to get most of the things on the "to do" list sorted first. At the moment the only place the library has been mentioned by myself is on the Arduino forum and I marked that post as being for "advanced users" until I can catch up with the support documentation. There are folk that have been using different platforms like Mega, Due and ESP8266 with a variety of displays, there are some subtle problems with different hardware boards that would be difficult to build into a single sketch without a lot of confusing #ifdef's, however as you have noted I have moved the common code into a sub-sketch to try to make things easier for users. Hopefully it will become clearer when the documentation is released.

Thanks for you feedback, it does help me see things from a different viewpoint.

[stevstrong]

Well, I am not familiar with Due and ESP, I didn't know about problems. I thought same functions work equally on each hardware. But I can imagine that each of these have some "specialities" how Arduino is set up for them. Same for STM32...😑 In this case it would then make sence to have different sketches for different controllers. Then no ifdefs, please. Otoh, i see no reson to separate sketches based on TFT libs (Bodmer or Adafruit compatible). Or is there also something that i am missing?

[Bodmer]

The Arduino team want all the bases covered before it is added to the library manager of the IDE. I now have Due, Mega, EESP8266 boards working with the IDE TFT l, Adafruit_GFX, UTFT and ILI9341_due libraries. Each has it's vagaries. UTFT was the trickiest to use as it bit-bashes the SPI link. So the examples menu will be reconfigured again once the testing an code commenting is done...

I have not tried it with a STM32 yet.