search

jasper-software / xv

XV Software
Other
26 stars 9 forks source link

fix for JPEGs from Nikon D7100 #3

Closed sthen closed 2 years ago

sthen commented 2 years ago

This is taken from a patch to xv that we have in OpenBSD ports originally written by @marcespie

Nikon D7100 generates JPEG files with multiple APP1 structs; they can be read OK with xv but if the file is edited, an attempt to save it will fail. Sample files (marked "camera original") showing the problem at https://www.kenrockwell.com/nikon/d7100.htm e.g. https://www.kenrockwell.com/nikon/d7100/sample-images/D71_0346.JPG

The original patch is longer (touching xv.h, xvdir.c, xvjpeg.c) as it adds a small framework for reporting error messages, I skipped that to keep this PR to just the relevant part, if they're of interest you can find the other patches in https://github.com/openbsd/ports/tree/master/graphics/xv/patches

mdadams commented 2 years ago

@sthen I am okay with your PR only including the most essential part of the patch from OpenBSD. (I am assuming that the remaining parts of the patch probably do not apply cleanly.) If someone has time later to add the other more "cosmetic" part of the patch, they can do so. At least with your PR in its current form, JPEG images from the Nikon camera model you mentioned will not cause problems. I'll hold off for a few days before doing the merge, however, in case @marcespie or @pghmcfc might have differing opinions and/or would like to express their thoughts on the matter.

sthen commented 2 years ago

@mdadams Thanks, that makes sense. Everything in the patches dir will apply cleanly to the current tree by the way (there's another diff for xvgif.c that we've been carrying for a while but I don't have any information about how to trigger the problem and haven't looked further into it).

mdadams commented 2 years ago

I am generally okay with all of the patches listed at https://github.com/openbsd/ports/tree/master/graphics/xv/patches being merged. I am okay with doing this all in this PR or in multiple PRs. Whichever approach is more convenient for you would be fine with me. Since the changes merge cleanly, it seems like a good idea to pick them up. (Note to @pghmcfc: Please feel free to comment if you have opinions one way or the other.)

mdadams commented 2 years ago

@sthen: Just as a point of clarification, would you prefer to merge all four of the changes in this PR? Or would you prefer to split them across multiple PRs?

sthen commented 2 years ago

I am happy with either, guess it would keep the commit history cleaner to separate them though - will send PRs for the others now.

mdadams commented 2 years ago

@sthen: I think that your approach of keeping the changes in separate PRs is probably the best way to go. I'll proceed to merge them. Thanks for the patches.