GNOME Bugzilla – Bug 764093
tag: exif: wrong parsing of ISO_SPEED and PHOTOGRAPHIC_SENSITIVITY tags
Last modified: 2018-11-03 11:45:37 UTC
Created attachment 324607 [details] [review] A proposed patch. The comments below take into account the standard description summary found in http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/EXIF.html According to the site above: 0x8827 is ISO or PhotographicSensitivity in EXIF2.3 0x8833 is ISOSpeed (dependent of SensitivityType in ExifISOTag) According to gstexif.c: 0x8827 is EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY 0x8833 is EXIF_TAG_ISO_SPEED Then we have following in gstexif.c@373 /* don't need the serializer as we always write the iso speed alone */ {GST_TAG_CAPTURING_ISO_SPEED, EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY, EXIF_TYPE_SHORT, 0, NULL, deserialize_add_to_pending_tags}, {GST_TAG_CAPTURING_ISO_SPEED, EXIF_TAG_SENSITIVITY_TYPE, EXIF_TYPE_SHORT, 0, serialize_sensitivity_type, deserialize_sensitivity_type}, {GST_TAG_CAPTURING_ISO_SPEED, EXIF_TAG_ISO_SPEED, EXIF_TYPE_LONG, 0, NULL, NULL}, Which means that EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY will have deferred parsing depending on the EXIF_TAG_SENSITIVITY_TYPE which is wrong as seen from the relation established before in the site. Instead, the Tag dependent on EXIF_TAG_SENSITIVITY_TYPE should be EXIF_TAG_ISO_SPEED which is not happening. This creates a bug that results in the ISO parameter never being parsed. A scenario where this happens is if Sensitivity Type is different than 3 (ISO Speed) and instead is 2 (Recommended Exposure Index). What will happen is that no EXIF_TAG_ISO_SPEED (0X8833) will exist because the type is 2 and the deferred EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY (0x8827) was discarded in the SensitivityType parsing because type is different than 3. No more ISO information can be retrieved from the file. So assuming we now correctly make the EXIF_TAG_ISO_SPEED dependent on the EXIF_TAG_SENSITIVITY_TYPE and leave the EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY correctly represent the GST_TAG_CAPTURING_ISO_SPEED the problem outlined before does not happen because a standard compliant exif field will not even have EXIF_TAG_ISO_SPEED if the EXIF_TAG_SENSITIVITY_TYPE is different than 3. For the example above where the EXIF_TAG_SENSITIVITY_TYPE is 2, the exif will have a RecommendedExposureIndex (0x8832) tag which is currently not handled. The EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY would then be parseable and not dependent on anything. The problem is that EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY is of size type EXIF_TYPE_SHORT which for a reason I do not understand does not have a function that parses shorts like parse_exif_<type>_tag. This happens with other types. I propose thus the following: static void parse_exif_short_tag(GstExifReader * reader, const GstExifTagMatch tag, guint32 count, guint32 offset, const guint8 * offset_as_data) This way in gstexif.c@parse_exif_long_tag we can parse EXIF_TYPE_SHORTs. With everything together the problem is solved on my side. I attach a proposed patch. Hoping I was clear and thankful for Gstreamer Paulo Neves
Created attachment 324608 [details] An image with an EXIF where the problem is ilustrated This image reproduces the scenario illustrated in the bug.
Comment on attachment 324607 [details] [review] A proposed patch. Can you write a real commit message, and include your real name as the author? A commit message should always be a one-line summary of the change, an empty line, and then a multi-line explanation of what this fixes and how/why.
Created attachment 324677 [details] [review] A proposed patch with correct message Patch proposal with corrected author and message.
Thanks for the patch and the updated commit. I can confirm that your patch extracts the iso-speed whereas it's not extracted without your patch. However, with your patch the unit test fails when simply serialising/deserialising a taglist with just an iso-speed tag to/from EXIF. Is that expect? If yes, the test needs to be fixed, otherwise the parsing code needs more work I guess? You can check this with: $ cd tests/check/ $ GST_CHECKS=test_exif_tags_serialization_deserialization make libs/tag.check-norepeat
Created attachment 324801 [details] [review] tests: add unit test for EXIF iso speed parsing Unit test based on your test file.
I cant really understand why the test fails. Yesterday I even got a 'Unexpected critical/warning: unknown tag 'capturing-iso-speed' which my grepping abilities did not pinpoint where it came from. Also, is it possible to run the tests with GDB? I found that the tests are all bash scripts and I can't easily attach to them. Also how does the gstexiftag choose the deserializer if there are multiple GST_TAG_CAPTURING_ISO_SPEED tags originating from different EXIF_TAGS? Is it a race?
If you run 'make help' in tests/check/ it will give you all the options. tests/check $ G_DEBUG=fatal_warnings make libs/tag.gdb runs it in gdb.
Not sure I agree with this patch. 0x8827 is EXIF_TAG_PHOTOGRAPHIC_SENSITIVITY 0x8833 is EXIF_TAG_ISO_SPEED And also 0x8830 SensitivityType And in this particular file we have 0x8827 ISO : 100 0x8830 Sensitivity Type : Recommended Exposure Index 0x8832 Recommended Exposure Index : 100 (No 0x8833 present) So, the reason why ISO is not extracted here is because we limited getting 0x8827 (photographic sensitivity) only when 0x8830 tells us that it is ISO Speed (value 3). 0x8827 only has a meaning after we parse 0x8830. That is why it is dependent on the former. Now, the real problem here is that we only accept 0x8830 when it is an ISO Speed. It can also be a SOS (Standard Output Sensitivity), REI (Recommended Exposure Index), ISO Speed or any combination of all. Quick reading about those different values seem to indicate they are all very similar, it seems REI might just be a non-official per-company setting that is similar to ISO on their sensors. Anyway, I think the question here is how to map other interpretations of SensitivityType to ISO, if possible.
I think the 0x8827 should never be dependent on anything. Because in previous standards it was a standalone tag. It's true that the REI and derivatives seem to be different names for ISO so the problem becomes one of collision rather than continuing to make the 0x8827 dependent on 0x8830. For the sake of your argument then, which one will become the ISO value, 0x8832 or 0x827, because they both appear in the same image at the same time, and it is not guaranteed they are the same.
(In reply to Paulo Neves from comment #9) > I think the 0x8827 should never be dependent on anything. Because in > previous standards it was a standalone tag. It's true that the REI and > derivatives seem to be different names for ISO so the problem becomes one of > collision rather than continuing to make the 0x8827 dependent on 0x8830. In case it had no 0x8830 we could just use 0x8827 as ISO as before but this file has also the 0x8830. So the question here is if we want to differentiate all of those values of 0x8830 into different tags or just map them to ISO. I don't have enough photographic sensor knowledge at the moment to decide that. Maybe someone out there has? > > For the sake of your argument then, which one will become the ISO value, > 0x8832 or 0x827, because they both appear in the same image at the same > time, and it is not guaranteed they are the same. No, it is not. In that case we can either put both in the tags as a list or just pick our favorite. Not sure what is the less worse option.
0x8830 SensitivityType --> (applies to EXIF:ISO tag) 0 = Unknown 1 = Standard Output Sensitivity 2 = Recommended Exposure Index 3 = ISO Speed 4 = Standard Output Sensitivity and Recommended Exposure Index 5 = Standard Output Sensitivity and ISO Speed 6 = Recommended Exposure Index and ISO Speed 7 = Standard Output Sensitivity, Recommended Exposure Index and ISO Speed and 0x8827 ISO int16u[n] ExifIFD I conclude that the 0x827 can even have many values, which would lead to extra tags. This extra tags are in my opinion superfluous and probably rare. About the difference between different sensitivity types somebody made some points here: http://www.dpreview.com/forums/thread/2838786 Again i think the simplest way to proceed without tag lists or new tags is to pick, of course ;), my favorite, 0x8827. I just need some help trying to clean up the code for the test to pass.
Created attachment 329877 [details] [review] Required changes and test cases Hello, I have a new proposed patch where I add new gst_tags related to the sensitivity of the sensor. I also corrected the test not passing as well as integrated the test patch provided by Tim-Muller. Additional checks were added to increase robustness of the patch. I also corrected what I think was a bug in the exif_long_parser where Big Endian values were not correctly read. This was not being tested I think. This was exactly what was making my code fail the tests in the big endian versions of the exif_short_parser code, which was a replica of the exif_long_parser. I am planning on launching a product that relies on these changes to be integrated with the main code, so if there are further requirements needed, please let me know and I will fix them.
Ping. Is there any eyes on the patch?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/260.