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

Omit JFIF header by default #36

Closed
wants to merge 1 commit into from
Closed

Omit JFIF header by default #36

wants to merge 1 commit into from

Conversation

ace-dent
Copy link

@ace-dent ace-dent commented Apr 7, 2014

Do not create JFIF header (APP0 block) by default. This saves 18 bytes per image. Fixes issue #18. Unlike original PR, this provides scope to fallback to default behaviour. Not the tidiest patch... I would prefer (once solution is field tested) to strip out all related code, for tidier code base. Utility of APP14 (Adobe) marker should also be investigated in future- is it actually useful for Colorspace data in context of web?

Do not create JFIF header (APP0 block) by default. This saves 18 bytes per image. Fixes issue #18.

Signed-off-by: Andrew <dent.ace@gmail.com>
@roytam1
Copy link

roytam1 commented Apr 8, 2014

APP14 is required by YCCK images (since there is no other method to distinguish YCCK image out of CMYK images without APP14)

@ace-dent
Copy link
Author

ace-dent commented Apr 8, 2014

@roytam1 - Thanks for the feedback.
Looking over the code, I thought the component_id was written into the SOF block (defined in jcparam.c and written by jcmarker.c). e.g. Component 4 id is Defined as '0x4B' (K) for CMYK or '4' for YCCK... but I guess that's not used (widely) externally?
Heuristics here: http://docs.oracle.com/javase/7/docs/api/javax/imageio/metadata/doc-files/jpeg_metadata.html#optcolor
Either way, for typical, compact web graphics I wouldn't expect use of these colorspaces... (where colour accuracy is vital, image size is secondary...). Let's keep APP14 intact :)

@ace-dent
Copy link
Author

ace-dent commented Apr 8, 2014

... Oh and the sourcecode refers to the APP14 Adobe Marker as 'optional'... :)

@roytam1
Copy link

roytam1 commented Apr 8, 2014

Component 4 id is Defined as '0x4B' (K) for CMYK or '4' for YCCK... but I guess that's not used (widely) externally?

in http://code.google.com/p/go/issues/detail?id=4500#c12 you can find CMYK image with compid=1,2,3,4

@ace-dent
Copy link
Author

ace-dent commented Apr 8, 2014

@roytam1 Thanks for the example. Interestingly the 'fix' for this doesn't use the APP14 marker, it also infers the color space by SOF (if no ICC profile):
https://github.com/nl5887/golang-image/blob/master/jpeg/reader.go#L142
(However, from a quick glance this may still fail...). Picking through those images, there's some nasty stuff, e.g. 'err1' file - APP14 defines YCCK, but has embedded ICC profile for CMYK... :-/
Clearly a bit complex, it seems sensible to hold off omitting the APP14 marker. Would be neat to get some more community feedback re. decoder approaches.

@bdaehlie bdaehlie modified the milestone: v2.0 May 7, 2014
@bdaehlie
Copy link
Contributor

bdaehlie commented May 9, 2014

Talked to some folks, we are going to go ahead and disable JFIF by default and leave an option to enable it. We can reverse course if we learn something about why this is problematic.

@bdaehlie bdaehlie removed this from the v2.0 milestone May 9, 2014
@bdaehlie
Copy link
Contributor

bdaehlie commented May 9, 2014

Upon further reflection, this win might not be worthwhile. Removing JFIF when it is expected could be considered a compatibility issue, and it doesn't really take up much space. Making it an option when it isn't with other encoders will cause some config headaches.

I'm going to close this out but am happy to re-consider later.

@bdaehlie bdaehlie closed this May 9, 2014
@ace-dent
Copy link
Author

@bdaehlie - I submitted this after a code review of main opensource decoding libraries in use (from web browsers to LibreOffice, etc.). Since cameras can produce jpegs with EXIF and no JFIF, I didn't find any implementations dependent on JFIF block for successful decoding. To verify this, I also discussed the proposal with various prominent individuals in the jpeg compression / hacking community. However, as discussed above, it was determined the APP14 marker is required for broad compatibility (although safe to remove for many decoders).

It seems a shame to limit compression based on certain decoding libraries. There should be some discussion of this re. #37 to define supported majority (and highlight any minority decoder implementations that wont be supported). We should also be making decisions based on code review (where decoding libraries have source code available)- rather than gut feel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants