Don't misinterpret DJI info maker note as DJI maker note #10470
Conversation
|
Just noticed I put master as target... I think this should've targetted 8.1 because fixing that metadata should be able to get parsed is a bugfix, while implementing a DJI info parser is a feature. |
| @@ -1287,6 +1290,7 @@ static const maker_note_type maker_note_array[] = { | |||
| { tag_table_VND_OLYMPUS, "OLYMPUS OPTICAL CO.,LTD", "OLYMP\x00\x01\x00", 8, 8, MN_ORDER_NORMAL, MN_OFFSET_NORMAL}, | |||
| { tag_table_VND_SAMSUNG, "SAMSUNG", NULL, 0, 0, MN_ORDER_NORMAL, MN_OFFSET_NORMAL}, | |||
| { tag_table_VND_PANASONIC, "Panasonic", "Panasonic\x00\x00\x00", 12, 12, MN_ORDER_NORMAL, MN_OFFSET_NORMAL}, | |||
| { REQUIRES_CUSTOM_PARSING, "DJI", "[ae_dbg_info:", 13, 13, MN_ORDER_MOTOROLA, MN_OFFSET_NORMAL}, | |||
There was a problem hiding this comment.
This [ae_dbg_info: is where I'm unsure. The exiftools docs lists a couple of DJI Info Tags, and ae_dbg_info is just one of those. Just checking for [ (like in my earlier POC) is likely too brittle.
There was a problem hiding this comment.
This [ae_dbg_info: is where I'm unsure. The exiftools docs lists a couple of DJI Info Tags, and ae_dbg_info is just one of those. Just checking for [ (like in my earlier POC) is likely too brittle.
I used https://exiftool.org/makernote_types.html to determine how to identify the info tag. Exiftool itself only checks for "[ae_dbg_info:" (https://github.com/exiftool/exiftool/blob/48df8aae22faa33d830dcf2ecdf406998b4d3849/lib/Image/ExifTool/Exif.pm#L3402) so that's why I thought that might suffice here. According to the makernote_types.html table, it seems like for two cameras it suffices to check for "[ae_dbg_info:".
Also, it might make sense to add a regression test, although that ~9MB picture provided by the bug reporter might be a bit large.
Yeah, I tried to shrink that image but that destroys the metadata in the editor I used. There's also some test images you can find on wikimedia.
There was a problem hiding this comment.
Exiftool itself only checks for "[ae_dbg_info:" (https://github.com/exiftool/exiftool/blob/48df8aae22faa33d830dcf2ecdf406998b4d3849/lib/Image/ExifTool/Exif.pm#L3402) so that's why I thought that might suffice here.
In this case, the patch is fine (I don't think we should attempt to be more thorough than exiftool.) And a test doesn't appear to be necessary, anyway.
|
Thanks. Shall I rebase this on 8.1? That would be my personal preference as it at least solves the blocking issue for the original issue reporter. |
Yes, please. Since this is a bug fix (we're not actually parsing the new info, but rather prevent that from stop parsing) it should target 8.1 |
The DJI and DJI info maker note both share the "DJI" make string. This caused the current code to try to interpret the DJI info maker note as a DJI maker note. However, the DJI info maker note requires custom parsing. Therefore, the misinterpretation actually caused the current code to believe that there was an unrecoverable error in the IFD for the maker note by returning false in the maker note parser. This in turn caused the inability to parse other EXIF metadata. This patch adds the identification of the DJI info maker note so that it cannot be misinterpreted. Since we don't implement custom parsing, it achieves this by setting the tag list to a special marker value (in this case the NULL pointer). When this marker value is detected, the function will just skip parsing the maker note and return true. Therefore, the other code will believe that the IFD is not corrupt. This approach is similar to handing an unrecognised maker note type (see the loop on top of exif_process_IFD_in_MAKERNOTE() which also returns true and treats it as a string). The end result of this patch is that the DJI info maker note is considered as unknown to the caller of exif_process_IFD_in_MAKERNOTE(), and therefore that the other EXIF metadata can be parsed successfully.
nielsdos
force-pushed
the
exif-feature
branch
from
30fe4e4 to
0051ba4
Compare
|
So the aporoval review got dismissed when I changed the base branch and rebased to 8.1. |
|
@Girgias Can you help here? If not, given that Christoph is currently unavailable it seems like you're best able to assess this, in that case feel free to merge when you see fit 🙂 |
|
Thanks! |
Partially handles GH-9397, please see my description below.
cc'ing @cmb69 since he did the analysis of the issue.
Description
The DJI and DJI info maker note both share the "DJI" make string.
This caused the current code to try to interpret the DJI info maker note
as a DJI maker note. However, the DJI info maker note requires custom
parsing. Therefore, the misinterpretation actually caused the current
code to believe that there was an unrecoverable error in the IFD for the
maker note by returning false in the maker note parser. This in turn
caused the inability to parse other EXIF metadata.
This patch adds the identification of the DJI info maker note so that it
cannot be misinterpreted. Since we don't implement custom parsing, it
achieves this by setting the tag list to a special marker value (in this
case the NULL pointer). When this marker value is detected, the function
will just skip parsing the maker note and return true. Therefore, the
other code will believe that the IFD is not corrupt.
This approach is similar to handing an unrecognised maker note type
(see the loop on top of exif_process_IFD_in_MAKERNOTE() which also
returns true and treats it as a string). The end result of this patch
is that the DJI info maker note is considered as unknown to the caller of
exif_process_IFD_in_MAKERNOTE(), and therefore that the other EXIF
metadata can be parsed successfully.
This patch also fixes some typos in the debug messages:
@xADDRESS->@0xADDRESS.Additional notes
We could implement such a DJI info parser in the future, but I think this is an acceptable stop-gap solution. At least the current data is not misinterpreted anymore. And if we encounter such problem cases in the future too, we could use the same solution.