1184246 - jpeg metadata parser doesn't read Exif data properly

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Description

[Hubert Figuiere [:hub]]

jpeg metadata parser doesn't read Exif data properly

It doesn't use the proper offset for read the data that's more than 4 bytes from the IFD.

Will attach a patch.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] hfiguiere:bug1184246-exif-fix-ifd-offset > mozilla-b2g:master

Comment 2

[Hubert Figuiere [:hub]]

Obviously we don't test this in the unit tests. I'll write some for bug 1184155 given the dependencies.

Comment 3

[Diego Marcos [:dmarcos]]

What image are you using to reproduce the bug? Is there anything special about it? I have a bunch of images I tried that are properly read on the demo:

http://mozilla-b2g.github.io/exif-parser/examples/

Comment 4

[Hubert Figuiere [:hub]]

I use whatever the flame camera generated. And re-read the spec.

Comment 5

[Hubert Figuiere [:hub]]

Also it is the code we have in shared, not that "exif-parser" we should probably switch to (bug 928612)

Comment 6

[Diego Marcos [:dmarcos]]

Were you able to load the image on the demo site?

http://mozilla-b2g.github.io/exif-parser/examples/

Can you attach the problematic image to this bug?

Comment 7

[Hubert Figuiere [:hub]]

Yes exif-parser do work properly with the file.

The reason we don't notice the bug above is that we don't load any Exif data that is bigger than 4 bytes.

Comment 8

[Hubert Figuiere [:hub]]

Attachment: IMG_0003.jpg

sample picture, generated by the Flame camera.

Comment 9

[Diego Marcos [:dmarcos]]

I think I misunderstanding the problem. If the image properly loads in the demo site what bug does your patch fix?

Comment 10

[Hubert Figuiere [:hub]]

My patch is for the code currently in gaia at 'shared/js/media/jpeg_metadata_parser.js'. Which is the other Exif parser - that is concatenated into the metadata_script.js in gallery.

But if I go and fix bug 928612 by moving to the newer parser - which looks more likely -, then this is obsolete.

Comment 11

[Diego Marcos [:dmarcos]]

Understood! Yes, go ahead an move to the new parser.

Comment 12

[Hubert Figuiere [:hub]]

bug 1184674 will take care of that. Will mark is dupe when the other lands and I have verified it all works (hint, to does)

Comment 13

[Hubert Figuiere [:hub]]

Comment on attachment 8634275 [details] [review]
[gaia] hfiguiere:bug1184246-exif-fix-ifd-offset > mozilla-b2g:master

After talking with David, we might want to keep this parser around for now. So let's get it fixed.

It still remains a no-op as we don't exercise that code path yet in our apps. (I have tested it though)

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8634275 [details] [review]
[gaia] hfiguiere:bug1184246-exif-fix-ifd-offset > mozilla-b2g:master

Thanks for finding and fixing this, Hub.  r+, but see my github comment: I'd prefer a simpler patch that just adds "+ 10" as a literal rather than declaring a new variable for this.

Comment 15

[Hubert Figuiere [:hub]]

Comment on attachment 8634275 [details] [review]
[gaia] hfiguiere:bug1184246-exif-fix-ifd-offset > mozilla-b2g:master

New review request: I use that "startOffset" where the constant 10 is used. here is what I wrote about it on github:


> Reading another patch I found the + 10 and thought the same.
>
> The reason I want to keep the +10 here is that the spec is vague about +10. 
> It says "This tags records the offset from the start of the TIFF header to 
> the position where the value itself is recorded." This is mostly "10" when it 
> is an Exif block in a JFIF APP1 segment. So, short term it doesn't matter. 
> Longer term, if we ever have a hardware that support raw files like some 
> Android or Lumia do we'll be happy to support plain TIFF file (includes DNG 
> which is TIFF/EP)
>
> I'll update the PR to take that into account.

This is the only change.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8634275 [details] [review]
[gaia] hfiguiere:bug1184246-exif-fix-ifd-offset > mozilla-b2g:master

r+ if you address the new comments on github.

I slightly prefer just using literal 10 everywhere, but am okay with this approach, too, as long as you remove the increment code which seems like a source of bugs.

Comment 17

[Hubert Figuiere [:hub]]

Merged
https://github.com/mozilla-b2g/gaia/commit/f75b12a281c1480df3354f7247ddec2c7bafe736