bitbank2 / TIFF_G4

A set of highly optimized functions for decoding and displaying 1-bpp CCITT G4 images
Apache License 2.0
22 stars 3 forks source link

TIFF_G4 1.2.0 broken on ESP8266 #3

Closed kleinesfilmroellchen closed 7 months ago

kleinesfilmroellchen commented 7 months ago

Library version 1.2.0 as available through current Arduino CLI library management.

Using a minified version of the example code which only tries to perform a no-op decode:

#include <TIFF_G4.h>
#include "arduino_logo.h"

#define DISPLAY_WIDTH 320
#define DISPLAY_HEIGHT 240

TIFFG4 tiff;
const uint16_t us2BppToRGB565[4] = { 0x0000, 0x528a, 0xa514, 0xffff };

void TIFFDraw(TIFFDRAW* pDraw)
{
    Serial.println("Another line");
} /* TIFFDraw() */

void setup()
{
    Serial.begin(115200);
    Serial.println("Starting...");
} /* setup() */

void loop()
{
    int x, cx;
    float fScale;

    if (tiff.openTIFF((uint8_t*)arduino_logo, (int)sizeof(arduino_logo), TIFFDraw)) {
        Serial.println("Initialized TIFF successfully.");
        Serial.printf("Size: %d x %d \n", tiff.getWidth(), tiff.getHeight());
        cx = tiff.getWidth(); // scale the image to fit the display
        fScale = (float)DISPLAY_WIDTH / (float)cx;
        // Use TIFF_PIXEL_1BPP for bitonal only, 2BPP for anti-aliased grayscale
        tiff.setDrawParameters(fScale, TIFF_PIXEL_2BPP, 0, 0, DISPLAY_WIDTH, DISPLAY_HEIGHT, NULL);
        Serial.println("Set draw parameters");
        tiff.decode();
        //   tiff.close();
    }
    while (true)
        yield();
} /* loop() */

(arduino_logo.h is exactly the same as in the example.)

on an ESP8266 (D1 Mini board, 80MHz) leads to the following exception:


--------------- CUT HERE FOR EXCEPTION DECODER ---------------

Exception (3):
epc1=0x4020166b epc2=0x00000000 epc3=0x00000000 excvaddr=0x4026394b depc=0x00000000

>>>stack>>>

ctx: cont
sp: 3ffffda0 end: 3fffffd0 offset: 0150
3ffffef0:  3fffff50 0000008b 00000068 000002e0  
3fffff00:  00000200 00000000 00000000 00000000  
3fffff10:  3fff24fc feef0001 feefeffe 3fff56dc  
3fffff20:  4026242b 00000001 00000020 401009d8  
3fffff30:  3ffefbc8 00000001 000002e0 3ffefbc8  
3fffff40:  00000050 00000022 00000000 3fff3188  
3fffff50:  00000000 3ffefc14 00006f4d 000002e0  
3fffff60:  3ffe8a66 00002d34 3ffefbc8 3fff28fc  
3fffff70:  00000002 3fff2efc 3ffefbc8 40202231  
3fffff80:  3fff3120 00000013 3fff3120 3fff3188  
3fffff90:  3fff3120 00000000 3ffefbc8 4020110c  
3fffffa0:  000000f0 00000000 3fff3120 402026d8  
3fffffb0:  3fffdad0 00000000 3fff3174 40202c90  
3fffffc0:  feefeffe feefeffe 3fffdab0 40100d51  
<<<stack<<<

--------------- CUT HERE FOR EXCEPTION DECODER ---------------

ESPExceptionDecoder says:

Exception 3: LoadStoreError: Processor internal physical address or data error during load or store
PC: 0x4020166b
EXCVADDR: 0x4026394b

Decoding stack results
0x401009d8: malloc(size_t) at Arduino15\packages\esp8266\hardware\esp8266\3.1.2\cores\esp8266\umm_malloc\umm_malloc.cpp line 912
0x40202231: Decode(TIFFIMAGE*) at Arduino\libraries\TIFF_G4\src/tiffg4.c line 1374
0x4020110c: loop() at generated_examples\adafruit_tiff_demo/adafruit_tiff_demo.ino line 38
0x402026d8: Print::println(char const*) at Arduino15\packages\esp8266\hardware\esp8266\3.1.2\cores\esp8266\Print.cpp line 238
0x40202c90: loop_wrapper() at Arduino15\packages\esp8266\hardware\esp8266\3.1.2\cores\esp8266\core_esp8266_main.cpp line 258

which is partially wrong (not sure why) but clearly there's a malloc issue in tiffg4.

The same series of library calls compile and work on Linux, producing correct output. Other TIFF files show the same behavior (and also work on Linux).

Removing the decode call makes it work, and in any case correct metadata is produced.

In general I have not found a way of making the library work on ESP8266. In most cases (in the context of complex real-world applications), even the openTIFF or openRAW call will hang the processor, eventually triggering a watchdog reset. Non-PROGMEM-data shows the same behavior. (The pgm_read macros can be used for all memory on Xtensa to my knowledge, but are slower for memory regions like RAM that do not require word access.) I'd be happy if you could investigate these issues, since the preprocessor checks, implementation code and documentation indicated to me that this library should work on ESP8266 and other 32-bit Xtensa/RISC-V boards.

kleinesfilmroellchen commented 7 months ago

In case this seems curious: Uncommenting the close() does not fix anything. In fact, AFAICT close should be a no-op given that there's no special callback for closing a memory input.

bitbank2 commented 7 months ago

Something is very odd about this problem - there are no references to malloc() in TIFF_G4; it's written specifically to not have any external dependencies. It definitely works on other MCUs including the whole ESP32 line. I normally don't use the ESP8266, but I tried to reproduce it here. There is either something wrong with the latest ESP8266 compiler or the Arduino build settings. I tried changing the image data to load from RAM (removed const and PROGMEM), no difference. There is nothing wrong with the C code, so I'm not going to invest time tracking down a problem specific to the ESP8266 or its compiler.

kleinesfilmroellchen commented 7 months ago

Okay then, that makes this library unusable for me. FWIW, very small TIFFs can be equally well compressed with some primitive RLE+entropy coding, which is what I'm doing now.

kleinesfilmroellchen commented 7 months ago

there are no references to malloc() in TIFF_G4; it's written specifically to not have any external dependencies.

I've read over the code a few times now (not sure if it's my "unfamiliarity" with C or the code's readability) and while it does indeed not call into any allocator (not even a user-provided one), a new question arises: Who initializes TIFFIMAGE::ucPixels, and why is this not done in the example? After all, somewhere we need some storage for one scanline. As far as I can tell it's not statically allocated either due to obviously unknown image dimensions at compile time.

I was considering that this may be an OOM issue in some cases, since my system is running under extremely low memory conditions (1-2KB heap free usually, a lot of resorting to the ESP8266's secondary IRAM heap). The example code, however, clearly should not suffer from OOM. Furthermore, OOM crashes are instant, while I usually observed watch dog timer resets.

Also, sidenote: Why are you closing this issue as "completed"? Nothing here is completed or fixed.

bitbank2 commented 7 months ago

Your criticisms of my behavior and my tool are not going to motivate me to do work that I don't need or want to do. Your comments indicate that don't understand how the code works. The TIFFIMAGE structure has a fixed sized, pre-allocated buffer to hold the current line of decoded pixels. There is a macro to control the size of this buffer and I set it large enough to handle most cases on small machines.

As far as closing this issue - there is only one way to do it. Closed in this case means I'm not going to fix/investigate something that isn't broken. If you think closed = completed, then that's your misguided interpretation.

I don't owe you or anyone else anything. I wrote code and shared it with the world for free. If you want me to fix the issue on the ESP8266, you can pay me. I don't use that chip for anything and if my project doesn't work on that chip, it doesn't mean that it's faulty, just that there's something odd about that environment.

bitbank2 commented 7 months ago

Okay then, that makes this library unusable for me. FWIW, very small TIFFs can be equally well compressed with some primitive RLE+entropy coding, which is what I'm doing now.

Actually no - RLE coding can get up to maybe 5:1 at best while TIFF G4 can get 20:1.

kleinesfilmroellchen commented 7 months ago

The TIFFIMAGE structure has a fixed sized, pre-allocated buffer to hold the current line of decoded pixels. There is a macro to control the size of this buffer and I set it large enough to handle most cases on small machines.

My images are 85 pixels in width; I presume that that would fit in the buffer.

As far as closing this issue - there is only one way to do it.

GitHub 1-2 years ago gained the ability to close issues as either completed or as "wontfix" (which will usually appear gray in the interface). The latter applies here, and this would help others looking for a solution for the same problem.

I don't owe you or anyone else anything. I wrote code and shared it with the world for free. If you want me to fix the issue on the ESP8266, you can pay me. I don't use that chip for anything

It seems like you took my criticisms personally, though I am at best criticising your code. Not even that really; I'm not good with raw C code and I'm certain that your code is efficient and working on AVR devices. I am aware that as an open-source maintainer you have no obligation to address any fixes that are requested; I am merely submitting a bug report with as much detail as possible to facilitate a fix. Since you aren't interested in that (and yes this seems like a nontrivial problem), my only further request is to document this issue properly, using GitHub's issue system as well as the main documentation if you're willing to do so. It has helped me many times with other Arduino libraries to know upfront which platforms have which limitations or are simply not supported.

bitbank2 commented 7 months ago

It seems like you took my criticisms personally, though I am at best criticising your code. Not even that really; I'm not good with raw C code and I'm certain that your code is efficient and working on AVR devices. I am aware that as an open-source maintainer you have no obligation to address any fixes that are requested; I am merely submitting a bug report with as much detail as possible to facilitate a fix. Since you aren't interested in that (and yes this seems like a nontrivial problem), my only further request is to document this issue properly, using GitHub's issue system as well as the main documentation if you're willing to do so. It has helped me many times with other Arduino libraries to know upfront which platforms have which limitations or are simply not supported.

Your criticisms were personal and full of misinformation and misunderstanding. Your comments were from a position of entitlement that I don't appreciate. I do appreciate bug reports and want my code to improve. This particular issue is 99.9% due to some oddity of that CPU and needs a special workaround that is not necessary on all other platforms. Therefore, it doesn't not deserve my full attention. If you find a workaround that doesn't disturb the code on other platforms, I'll consider adding it.

bitbank2 commented 7 months ago

I was curious about this issue, so I debugged it further - the crash was due to the ESP8266 FLASH memory addressing; it requires pgm_read_byte() since it has overlapping RAM/ROM addresses. I added a fix and pushed it to the repo. I'll do a new release if you confirm that it works.