Handling of GPSProcessingMethod not correct

[nicofooo]

Hello,
after bug #1046, you have well added the support of the GPSProcessingMethod tag.
At that time I tested (#1046 (comment)) your patch and it worked fine then.

However, now that exiv2 0.27.3 is officially available, it does not work well for some values of the GPSProcessingMethod:

• if GPSMethod = "MANUAL" => works fine
• if GPSMethod = "GPS" => does not work: displays "binary comment" instead of "GPS"

Look at the output of exiv2 with different versions:

0.27.2

$ exiv2 -pv -g GPSProcessingMethod/i 20190815_124*
20190815_124300.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  65 83 67 73 73 0 0 0 77 65 78 85 65 76
20190815_124441.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  65 83 67 73 73 0 0 0 71 80 83 0 0 0

0.27.2 + patch from #1046

$ exiv2 -pv -g GPSProcessingMethod/i 20190815_124*
20190815_124300.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  charset="Ascii" MANUAL
20190815_124441.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  charset="Ascii" GPS

This is OK !

0.27.3

$ exiv2 -pv -g GPSProcessingMethod/i 20190815_124*
20190815_124300.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  charset=Ascii MANUAL
20190815_124441.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  charset=Ascii binary comment

This is NOK !

[clanmills]

Groan! I can't think why v0.27.3 isn't the same as 0.27.2 + the patch in #1046. I will investigate.

I assume "NOK" means "Thank You, Robin, for all your unpaid work on Exiv2".

[clanmills]

I've remembered what this is about. #1258 We've had trouble in the test suite with binary characters being written to std-out.

I have a new contributor in China working to replace bash_tests with code written in python. When that's complete, we may decide that binary output in std-out is OK. So, please be patient, binary output is being reviewed for v0.27.4 (or v0.28).

[nicofooo]

I assume "NOK" means "Thank You, Robin, for all your unpaid work on Exiv2".

It just means that it is not the expected result.
I appreciated your work on previous issue #1046 and I'm sure this issue will be solved as well ;)

[clanmills]

Can you attach your file to this issue, please?

[clanmills]

I think there are binary characters in your file:

20190815_124441.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  charset="Ascii" GPS

The length is 14, however it should be 11. Comments have an 8 byte header to store the charset PLUS the number of bytes necessary for the string. So, charset=Ascii GPS should be 11, not 14. What are the other bytes in your file? I suspect they are NUL.

519 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ cp test/data/FurnaceCreekInn.jpg  .
520 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -g Exif.GPSInfo.GPSProcessingMethod FurnaceCreekInn.jpg 
Exif.GPSInfo.GPSProcessingMethod             Undefined  18  charset=Ascii HYBRID-FIX
521 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -M"set Exif.GPSInfo.GPSProcessingMethod charset=Ascii GPS" FurnaceCreekInn.jpg 
522 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -g Exif.GPSInfo.GPSProcessingMethod FurnaceCreekInn.jpg 
Exif.GPSInfo.GPSProcessingMethod             Undefined  11  charset=Ascii GPS
523 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -M"set Exif.GPSInfo.GPSProcessingMethod charset=Ascii GPS\tTAB" FurnaceCreekInn.jpg 
524 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -g Exif.GPSInfo.GPSProcessingMethod FurnaceCreekInn.jpg 
Exif.GPSInfo.GPSProcessingMethod             Undefined  15  charset=Ascii binary comment
525 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ 

The obvious workaround is to fix your file:

$ exiv2 -M"set Exif.GPSInfo.GPSProcessingMethod charset=Ascii GPS" 20190815_124441.jpg 

I think we've discovered a bug in 0.27.2 concerning the reporting of the value of Exif.GPSInfo.GPSProcessingMethod when the comment has embedded binary characters and it has been fixed in 0.27.3.

The following code from src/value.cpp is responsible for the "binary comment". If my hunch that your "GPS" value is nul padded ("GPS\0\0\0"), I could add code to say "OK, you're a binary, but I'll print you if you are nul padded.".

    std::string CommentValue::comment(const char* encoding) const
    {
        std::string c;
        if (value_.length() < 8) {
            return c;
        }
        c = value_.substr(8);
        if (charsetId() == unicode) {
            const char* from = encoding == 0 || *encoding == '\0' ? detectCharset(c) : encoding;
            convertStringCharset(c, from, "UTF-8");
        } else {
            // charset=undefined reports "binary comment" if it contains non-printable bytes
            // this is to ensure no binary bytes in the output stream.
            if ( isBinary(c) ) {
                c = "binary comment" ;
            }
        }
        return c;
    }

I think such a fix will make you, me, and the test suite happy. And, Leo's "all-new python test suite" doesn't need to know. If the 3 mystery bytes are nul, we can quickly and easily deal with this. If the 3 mystery bytes are not nul, I'll have to investigate a little more.

So...... please send me 20190815_124441.jpg.

[nicofooo]

Thanks for investigating this so quickly.
I think your assumption is correct, based on what 0.27.2 reported (see my first message):

GPSProcessingMethod         Undefined  14  65 83 67 73 73 0 0 0 71 80 83 0 0 0
                                --->       A  S  C  I  I  . . . G  P  S  0 0 0 

You can check with the photo [here].

[clanmills]

Thanks for sending '20190815_124441.jpg' and your observation:

GPSProcessingMethod         Undefined  14  65 83 67 73 73 0 0 0 71 80 83 0 0 0
                                --->       A  S  C  I  I  . . . G  P  S  0 0 0

Tomorrow , I will submit a PR (destination v0.27.4). For you, the immediate workaround is to remove the binary bytes in that file with:

$ exiv2 -M"set Exif.GPSInfo.GPSProcessingMethod charset=Ascii GPS" 20190815_124441.jpg

This team-work stuff really works. I don't know why it was never used when I was at work. (I'm 69 and very happily retired).

[clanmills]

I've submitted a fix #1267.

The PR includes test suite changes. The code change is in src/value.cpp

    std::string CommentValue::comment(const char* encoding) const
    {
        std::string c;
        if (value_.length() < 8) {
            return c;
        }
        c = value_.substr(8);
        if (charsetId() == unicode) {
            const char* from = encoding == 0 || *encoding == '\0' ? detectCharset(c) : encoding;
            convertStringCharset(c, from, "UTF-8");
        }
        bool bAscii = charsetId() == undefined || charsetId() == ascii ;
        // # 1266 Remove trailing nulls
        if ( bAscii && c.find('\0') != c.std::string::npos) {
            c = c.substr(0,c.find('\0'));
        }
        // return "binary comment" if results contains non-printable bytes
        // this ensures no binary bytes in the output stream.
        if ( bAscii && isBinary(c) ) {
            c = "binary comment" ;
        }
        return c;
    }

And with your file, I see:

619 rmills@rmillsmm-local:~/gnu/github/exiv2/0.27-maintenance/build $ bin/exiv2 -g Process  ~/Downloads/20190815_124441.jpg 
Exif.GPSInfo.GPSProcessingMethod             Undefined  14  charset=Ascii GPS
620 rmills@rmillsmm-local:~/gnu/github/exiv2/0.27-maintenance/build $ bin/exiv2 -g Process  ~/Downloads/20190815_124441.jpg | dmpf -
       0        0: Exif.GPSInfo.GPSProcessingMethod  ->  45 78 69 66 2e 47 50 53 49 6e 66 6f 2e 47 50 53 50 72 6f 63 65 73 73 69 6e 67 4d 65 74 68 6f 64
    0x20       32:              Undefined  14  char  ->  20 20 20 20 20 20 20 20 20 20 20 20 20 55 6e 64 65 66 69 6e 65 64 20 20 31 34 20 20 63 68 61 72
    0x40       64: set=Ascii GPS.                    ->  73 65 74 3d 41 73 63 69 69 20 47 50 53 0a
621 rmills@rmillsmm-local:~/gnu/github/exiv2/0.27-maintenance/build $ 

If you are happy with this change, please close this issue.

[nicofooo]

Thanks a lot. I have tested your fix: it works fine! So I close this issue.

Note: there is a typo in your comment in the code (1 "s" is too much):
// return "binary comment" if **results contains** non-printable bytes

[clanmills]

Yes, I noticed that after I submitted the code. Pretty harmless, no? I'm sure I'll be back here again soon. Somebody will complain about something else that I've never thought or heard about!

I groaned when this showed up on Thursday. Turned out to be simple. Even better, nothing wrong in #1046. It's a related, yet different matter. I have a new contributor in China working with me. At the moment we're working on converting our bash test scripts to python as bash isn't windows friendly. I'd like him to investigate the unicode (and/or jis) support in those tags.

[clanmills]

@nicofooo Heads Up I submitted a PR #1432 concerning the output of comments. As Exiv2 passes the test suite, I think it's unlikely that #1432 will result in pain for you. However, I'm making you aware.

[nicofooo]

Hello @clanmills, thanks for letting me know.
I have tested #1432: everything is still working fine with GPSProcessingMethod for me.

0.27.4.9 #1432

$ exiv2 -pv -g GPSProcessingMethod/i 20190815_124*
20190815_124300.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  charset=Ascii MANUAL
20190815_124441.jpg   0x001b GPSInfo      GPSProcessingMethod         Undefined  14  charset=Ascii GPS

Thanks again for your work ;-)

[clanmills]

Thank You for getting back to me. Very pleased to hear that I haven't caused you pain.