Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backward ABI compatibility with libjpeg-turbo #118

Merged
merged 6 commits into from
Nov 12, 2014
Merged

Backward ABI compatibility with libjpeg-turbo #118

merged 6 commits into from
Nov 12, 2014

Conversation

dcommander
Copy link
Contributor

This patch moves the mozjpeg-specific parameters into the opaque master struct and implements a set of accessor functions to get/set them. This restores backward ABI compatibility with libjpeg/libjpeg-turbo. Additionally, a second patch fixes a minor build issue discovered with Visual C++ while working on the backward ABI compatibility stuff (Visual C++ doesn't support C99.)

…ng the mozjpeg-specific parameters into the opaque jpeg_comp_master struct and implementing generic accessor functions for getting/setting those parameters. These functions can be used upstream, if the need for them arises in libjpeg-turbo, and they can also be easily extended to cover future extensions to the decompressor. Note that, in order to use jpeg_comp_master as a repository for extension parameters, cinfo->master is now allocated within the body of jpeg_CreateCompress(). It is later re-allocated in jinit_c_master_control(), because that function (and others in jcmaster.c) use an extended form of jpeg_comp_master, but the existing extension parameters are copied into the new master instance. Similar modifications would need to be made to the decompressor to support the same type of extension framework.
…ort C99) -- at least not the version of MSVC++ I'm using.)
…unctions.

The ABI compatibility feature was developed by the current maintainer of
libjpeg-turbo with an eye toward eventual inclusion in libjpeg-turbo (once
other features are added to libjpeg-turbo that necessitate the inclusion.)
Thus, it is easy to ensure that the DLL function ordinals will be synchronized
between libjpeg-turbo and mozjpeg.  However, it still makes sense to allow for
a little bit of breathing room, just in case.  Thus, this patch uses ordinals
starting at 200 for the accessor functions.  It would probably make sense to
start the equivalent decompressor get/set functions at ordinal 300, once they
are implemented.
…jpeg is one higher (because it introduces new functions into the API)
@kornelski
Copy link
Member

Thanks @dcommander for this. This is a quite nice solution.

I wanted to add extension that specifies per-block quality, and this would require giving encoder multiple 2-dimensional arrays (#116), but I don't see how to do that with jpeg_c_set_*_param functions.

Should this API be extended to support pointer parameters (or virtual arrays?) as well?

Alternatively, maybe I should just define a custom function and declare it with __attribute__((weak_import, weak))?

@dcommander
Copy link
Contributor Author

Using weak_import is not a good idea, because it's unlikely to work with non-ELF targets. I would suggest just adding a new API function (or functions) that is specific to your extension. When Josh and Frank and I discussed this, we came to the conclusion that it was best to provide generic accessor methods only for simple datatypes-- that is, for cases in which the function is likely to be reusable for getting/setting multiple parameters. It is a no-brainer to add, for instance, a double or long accessor type, if needed, based on the existing template. But in your case, the probability of reusability is low, and it's more important to have a function that is tailored to your specific needs.

fbossen added a commit that referenced this pull request Nov 12, 2014
Backward ABI compatibility with libjpeg-turbo
@fbossen fbossen merged commit 1c5a481 into mozilla:master Nov 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants