Closed
Bug 1184246
Opened 9 years ago
Closed 9 years ago
jpeg metadata parser doesn't read Exif data properly
Categories
(Firefox OS Graveyard :: Gaia::Shared, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
FxOS-S4 (07Aug)
People
(Reporter: hub, Assigned: hub)
References
Details
Attachments
(2 files)
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•9 years ago
|
||
Assignee | ||
Comment 2•9 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•9 years ago
|
Attachment #8634275 -
Flags: review?(dmarcos)
Comment 3•9 years ago
|
||
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/
Updated•9 years ago
|
Flags: needinfo?(hub)
Assignee | ||
Comment 4•9 years ago
|
||
I use whatever the flame camera generated. And re-read the spec.
Flags: needinfo?(hub)
Assignee | ||
Comment 5•9 years ago
|
||
Also it is the code we have in shared, not that "exif-parser" we should probably switch to (bug 928612)
Comment 6•9 years ago
|
||
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•9 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•9 years ago
|
||
sample picture, generated by the Flame camera.
Comment 9•9 years ago
|
||
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•9 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)
Assignee | ||
Comment 12•9 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•9 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 14•9 years ago
|
||
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•9 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 16•9 years ago
|
||
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•9 years ago
|
||
Merged https://github.com/mozilla-b2g/gaia/commit/f75b12a281c1480df3354f7247ddec2c7bafe736
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•9 years ago
|
Target Milestone: --- → FxOS-S4 (07Aug)
You need to log in
before you can comment on or make changes to this bug.
Description
•