ABI compatibility question

[gidapataki]

Hi!

We are considering using your fork in our project, but it would be beneficial to maintain ABI compatibility with mainline libjpeg-turbo. We've seen the discussion on sourceforge, and we are interested if you have done any investigation about what it would take to achieve this.

Thanks in advance!

[komakai]

Thanks for your question. Here is a quick summary of what I think you would need to do to maintain binary compatibility

In jpeg_decompress_struct the following members would need to be moved to the end of the structure:

• pm_dummy_pass
• lowmem_progressive_decode
• scancontextctl

In jpeg_memory_mgr the extra parameter added to request_virt_barray breaks binary compatibility. You would need to do something like:

• rename request_virt_barray to request_virt_barray2 and move the declaration to the end of jpeg_memory_mgr
• restore the original declaration of request_virt_barray to its original position
• have the implementation request_virt_barray simply forward the call to request_virt_barray2 passing the final parameter as FALSE

The ENABLE_MEMORY_TEST mechanism when enabled also breaks binary compatibility. You don't need to do anything to fix this - it just means won't be able to run the memory test with a binary compatible version of the library.

[gidapataki]

I'm afraid that's more tricky than that: jpeg_decompress_struct is a public struct which is allocated by the client. If a client library was compiled with a different size of this struct, then it won't be compatible with the new function trying to set those fields at the end of the structure.

It would be best if these structs were not touched, which potentially means that the lowmem-related fields have to be private (not sure where to put), or the API should be extended, and using the lowmem path would require updating the client code (but this is less preffered).

Does this make sense?

[komakai]

jpeg_decompress_struct is a public struct which is allocated by the client.

Ahh - good point. And so are jpeg_memory_mgr and jpeg_source_mgr. Maybe the following would work: Create a new private structure for all the values introduced in lowmem progressive jpeg fork

• pm_dummy_pass
• lowmem_progressive_decode
• scancontextctl
• request_virt_barray2 (function pointer to the new implementation of request_virt_barray)
• check_seekable
• get_src_offset
• restore_src

Create a mapping from jpeg_decompress_struct to the private structure (a simple hashtable would be fine for this). Inside jpeg_create_decompress create an instance of the private structure and add an entry to the map (from the j_decompress_ptr parameter passed into jpeg_create_decompress to the instance of the private structure you created). Inside jpeg_destroy_decompress free the instance of the private structure and remove it from the map.

[gidapataki]

Hmm, it could work, but it introduces other problems like if there is a global hashtable then that needs to be threadsafe, plus we don't know what memorymanager we can use there. An other consideration is that check_seekable() and its companions must be provided by the client for custom source managers.

All in all, I'm leaning towards having new API functions that enable the low-memory codepath.

[komakai]

needs to be threadsafe

right - it would need to be thread-safe. So the hashtable methods would need to be protected with a mutex or other similar locking mechanism

we don't know what memorymanager we can use

I think it would be fine just to use the jpeg_memory_mgr memory management

check_seekable() and its companions must be provided by the client for custom source managers

I think this is a separate issue to ABI compatibility - or am I missing something?

[gidapataki]

Ah ok, I assumed a custom memory manager can be used and then there could have been a clash between hash items allocated by different mem managers, but this is not an issue then.

The check_seekable() also breaks ABI compatibility because it's a new field on the public jpeg_source_mgr. The client may provide a jpg_source_mgr, for which it didn't set the check_seekable and then the call in jdmaster.c:571 will be undefined behavior.

For me it's clear now that a library compiled with an older version cannot take the low-memory path implicitly, which is ok. The low-memory path also should not be default, because it introduces a tradeoff: higher mem usage vs potentially slower reads from source (jumping back and forth in the source may have a performance penalty based on the source's implementation). I'm not sure what happens if the client really needs the pixel data progressively, but the full buffer needs to be allocated in that case anyway.