jpeg metadata parser doesn't read Exif data properly

RESOLVED FIXED in FxOS-S4 (07Aug)

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: hub, Assigned: hub)

Tracking

unspecified
FxOS-S4 (07Aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
Created attachment 8634275 [details] [review]
[gaia] hfiguiere:bug1184246-exif-fix-ifd-offset > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Blocks: 1184155
(Assignee)

Comment 2

3 years ago
Obviously we don't test this in the unit tests. I'll write some for bug 1184155 given the dependencies.
Assignee: nobody → hub
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Attachment #8634275 - Flags: review?(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/
Flags: needinfo?(hub)
(Assignee)

Comment 4

3 years ago
I use whatever the flame camera generated. And re-read the spec.
Flags: needinfo?(hub)
(Assignee)

Comment 5

3 years ago
Also it is the code we have in shared, not that "exif-parser" we should probably switch to (bug 928612)
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?
Flags: needinfo?(hub)
(Assignee)

Comment 7

3 years ago
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.
Flags: needinfo?(hub)
(Assignee)

Comment 8

3 years ago
Created attachment 8634401 [details]
IMG_0003.jpg

sample picture, generated by the Flame camera.
I think I misunderstanding the problem. If the image properly loads in the demo site what bug does your patch fix?
Flags: needinfo?(hub)
(Assignee)

Comment 10

3 years ago
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.
Flags: needinfo?(hub)
Understood! Yes, go ahead an move to the new parser.
Flags: needinfo?(hub)
(Assignee)

Comment 12

3 years ago
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)
Flags: needinfo?(hub)
(Assignee)

Comment 13

3 years ago
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)
Attachment #8634275 - Flags: review?(dmarcos) → review?(dflanagan)
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.
Attachment #8634275 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 15

3 years ago
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.
Attachment #8634275 - Flags: review?(dflanagan)
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.
Attachment #8634275 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 17

3 years ago
Merged
https://github.com/mozilla-b2g/gaia/commit/f75b12a281c1480df3354f7247ddec2c7bafe736
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Target Milestone: --- → FxOS-S4 (07Aug)
You need to log in before you can comment on or make changes to this bug.