Closed
Bug 891030
Opened 11 years ago
Closed 7 years ago
[Gallery] [User Story] Edit image orientation to correct for tilt sensor inaccuracies
Categories
(Firefox OS Graveyard :: Gaia::Gallery, defect)
Tracking
(tracking-b2g:backlog)
RESOLVED
WONTFIX
tracking-b2g | backlog |
People
(Reporter: skasetti, Assigned: dmarcos)
References
Details
(Whiteboard: [ucid:Media28, ft:media], [MEDIA_TRIAGED] [priority])
Attachments
(4 files)
User Story:
As a user, I want the ability to rotate the orientation of the image using the image viewer or editor to correct any tilt sensor inaccuracies
Reporter | ||
Updated•11 years ago
|
Whiteboard: [ucid:Media 28]
Updated•11 years ago
|
Summary: [Gallery] [User Story] Rotate image orientation to correct for tilt sensor inaccuracies → [Gallery] [User Story] Edit image orientation to correct for tilt sensor inaccuracies
Comment 1•11 years ago
|
||
We made a decision for 1.2 to allow editing the image rotation by editing the orientation metadata in the EXIF header.
The EXIF header is stored in the JPEG file as an APP1 segment that contains a complete TIFF file, possibly including a JPEG-compressed thumbnail. Regarding orientation, the TIFF orientation tag (274d, 112h) has a SHORT (2-byte) value that determines which corner of the image is (0,0), and in what direction image data progresses; eight valid values are defined, four of which rotate the image, and four of which rotate-and-mirror the image.
Here is one implementation of reading EXIF data in JS:
https://github.com/bennoleslie/jsjpegmeta/blob/master/jpegmeta.js
http://benno.id.au/blog/2009/12/30/html5-fileapi-jpegmeta
In short, the Gallery would read the JPEG to be modified, find the TIFF orientation field in the EXIF header, and write the new desired value to the file.
CW--> { 1, 8, 3, 6 } <-- CCW
See:
http://sylvana.net/jpegcrop/exif_orientation.html
Heavy references:
- EXIF 2.2: http://www.exif.org/Exif2-2.PDF
- TIFF 6: http://www.exif.org/TIFF6.pdf
Comment 2•11 years ago
|
||
Wikipedia also has a good overview of EXIF and issues related to it:
http://en.wikipedia.org/wiki/Exif
Comment 3•11 years ago
|
||
dhylands, does DeviceStorage support in-place editing of files? Or does a single-byte change require a read-modify-write cycle?
Flags: needinfo?(dhylands)
Comment 4•11 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #3)
> dhylands, does DeviceStorage support in-place editing of files? Or does a
> single-byte change require a read-modify-write cycle?
There are currently no APIs which would support in-place editing.
I think bug 859696 needs to land first, and I think that needs child support for file handles.
Flags: needinfo?(dhylands)
Comment 5•11 years ago
|
||
Draft UI spec for discussion.
Updated•11 years ago
|
Comment 6•11 years ago
|
||
This bug also depends on bug 900425 which will enable Gallery to respond to "soft rotation" via EXIF orientation data. If gallery can display images that are "soft rotated" then rotating them becomes a relatively trivial matter of setting an EXIF flag.
Depends on: 900425
Updated•11 years ago
|
Assignee: nobody → dmarcos
Assignee | ||
Comment 8•11 years ago
|
||
Rob. I have a question concerning the UX design. Rotating the image is actually editing the metadata of the image. It makes sense to me that button for rotation should be included in the edition menu instead of the navigation screen
Assignee | ||
Comment 9•11 years ago
|
||
Patryk. We need a new asset for the rotation button
Flags: needinfo?(padamczyk)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(rmacdonald)
Comment 10•11 years ago
|
||
Peter, can you please provide Diego with the new action icon? Thanks.
Flags: needinfo?(padamczyk) → needinfo?(pla)
Comment 11•11 years ago
|
||
Attached is zip containing:
1) A screenshot of the new icon inside the app
2) 3 sizes of the icon (30x30 for the default 320x480 screen, 45x45 for 1.5x res, and 60x60 for 2x res)
Flags: needinfo?(pla)
Comment 12•11 years ago
|
||
Rob will address the button, but I'm just noting that this issue should not be blocking.
Comment 13•11 years ago
|
||
(In reply to Diego Marcos from comment #8)
> Rob. I have a question concerning the UX design. Rotating the image is
> actually editing the metadata of the image. It makes sense to me that button
> for rotation should be included in the edition menu instead of the
> navigation screen
Hi Diego...
Although rotating an image is technically image editing, we chose not to put image rotation in the image editing menu. The reason is that this is a common use case that we wanted to make easily accessible whereas image editing is more geared toward power-features.
Also, to confirm, with edited images (using the edit menu) we save a copy of the original. In the image rotation case, only the rotated version should be displayed.
- Rob
Flags: needinfo?(rmacdonald)
Assignee | ||
Comment 14•11 years ago
|
||
Is rotating an image that common of a use case? Assuming that's the case, the approach is still confusing.
Users are going to tap the rotation button to change how the image is displayed without knowing that the file is actually modified. The rotation is going to be preserved leading to unexpected results when sharing with other users or copying the image from the sdcard to a different device. It has to be a clear distinction between the edition and view modes. In the edition mode the user has to tap a button to confirm the changes making it clear that the image is modified. Both galleries in Android and iOS implement rotation in the same way. Attached screenshot of gallery on Android. I don't attach screenshot of iOS because I don't know if we're allowed to.
Assignee | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
(In reply to Diego Marcos from comment #14)
> Is rotating an image that common of a use case? Assuming that's the case,
> the approach is still confusing.
>
> Users are going to tap the rotation button to change how the image is
> displayed without knowing that the file is actually modified. The rotation
> is going to be preserved leading to unexpected results when sharing with
> other users or copying the image from the sdcard to a different device. It
> has to be a clear distinction between the edition and view modes. In the
> edition mode the user has to tap a button to confirm the changes making it
> clear that the image is modified. Both galleries in Android and iOS
> implement rotation in the same way. Attached screenshot of gallery on
> Android. I don't attach screenshot of iOS because I don't know if we're
> allowed to.
Hi Diego - Thanks for clarifying and you raise some really good points. Can you tell me a bit more about the unexpected results users might encounter? Is it that the previous metadata is wiped? Also, what's the cut off for making changes? - Rob
Flags: needinfo?(dmarcos)
Comment 17•11 years ago
|
||
Currently being worked on for 1.2 release
blocking-b2g: koi? → ---
Whiteboard: [ucid:Media 28] → [ucid:Media 28] [MEDIA_TRIAGED]
Target Milestone: --- → 1.2 FC (16sep)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(dmarcos)
Assignee | ||
Updated•11 years ago
|
Updated•11 years ago
|
Target Milestone: 1.2 FC (16sep) → 1.3 Sprint 3 - 10/25
Comment 18•11 years ago
|
||
Diego Marcos added a comment in Pivotal Tracker:
Waiting for dependecies resolution before landing a patch for this
Updated•11 years ago
|
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
Assignee | ||
Comment 19•11 years ago
|
||
We should revisit the UX for this bug. With the addition of the auto enhancement feature we have 5 buttons in the bottom bar of the edit mode. Adding a sixth one would make the buttons too small. Where should we put the rotation?
Flags: needinfo?(rmacdonald)
Flags: needinfo?(pla)
Comment 20•11 years ago
|
||
Following up on a call with Peter and Diego, the latest version of the spec is located in this folder:
https://app.box.com/s/ke19803iqja2zj31rjur
Flags: needinfo?(rmacdonald)
Comment 21•11 years ago
|
||
It's urgent for us in v1.2 branch. Could you finish it as soon as possible. Thanks.
Comment 22•11 years ago
|
||
(In reply to Jesse from comment #21)
> It's urgent for us in v1.2 branch. Could you finish it as soon as possible.
> Thanks.
We aren't taking features at this point in the 1.2. Blocking-
blocking-b2g: koi? → -
Comment 23•11 years ago
|
||
Clearing my needsinfo. I sync'd up yesterday with Diego and Rob and we will be going with the spec referenced in Comment 20 by Rob.
Flags: needinfo?(pla)
Updated•11 years ago
|
Whiteboard: [ucid:Media 28] [MEDIA_TRIAGED] → [ucid:Media 28] [MEDIA_TRIAGED][1.3:p2]
Comment 24•11 years ago
|
||
Hi, I've posted the icon assets for Gallery Edit Mode to this bug https://bugzilla.mozilla.org/show_bug.cgi?id=933066
Assignee | ||
Comment 25•11 years ago
|
||
Thanks Peter. You are providing icons on 1x and 1.5x resolution. In the gallery we currently have all the assets in 1x, 1.5x and 2x. Can we have these new icons also in 2x resolution?
Flags: needinfo?(pla)
Comment 26•11 years ago
|
||
Hey Diego,
According to Patryk we are no longer supporting 2x resolution icons.
Flags: needinfo?(pla)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #8337151 -
Flags: review?(dflanagan)
Comment 28•11 years ago
|
||
Update whiteboard tag to follow format [ucid:{id}, {release}:p{1,2}, ft:{team-id}]
Whiteboard: [ucid:Media 28] [MEDIA_TRIAGED][1.3:p2] → [ucid:Media28,1.3:p2], [MEDIA_TRIAGED]
Comment 29•11 years ago
|
||
Update whiteboard tag to follow format [ucid:{id}, {release}:p{1,2}, ft:{team-id}]
Whiteboard: [ucid:Media28,1.3:p2], [MEDIA_TRIAGED] → [ucid:Media28, 1.3:p2, ft:media], [MEDIA_TRIAGED]
Comment 30•11 years ago
|
||
NI Fabrice (1.3 sheriff) to get his take on landing this feature for 1.3 once we have all the reviews completed this week.
Flags: needinfo?(fabrice)
Comment 31•11 years ago
|
||
(In reply to Hema Koka [:hema] from comment #30)
> NI Fabrice (1.3 sheriff) to get his take on landing this feature for 1.3
> once we have all the reviews completed this week.
Btw - we might want to do a QA exploratory test pass on this to get a clear picture of quality risk here. Marcia is out sick today, but we should be able to do this tomorrow if you like.
Comment 32•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #31)
> (In reply to Hema Koka [:hema] from comment #30)
> > NI Fabrice (1.3 sheriff) to get his take on landing this feature for 1.3
> > once we have all the reviews completed this week.
>
> Btw - we might want to do a QA exploratory test pass on this to get a clear
> picture of quality risk here. Marcia is out sick today, but we should be
> able to do this tomorrow if you like.
Thanks Jason! Please work with dmarcos on what you need to get the testing done.
Comment 33•11 years ago
|
||
Okay. Marcia - Can you look into this?
Flags: needinfo?(mozillamarcia.knous)
Comment 34•11 years ago
|
||
What is the status on this for 1.3? Thank you.
Comment 35•11 years ago
|
||
Comment on attachment 8337151 [details] [review]
Pull Request
r- because:
1) the code rewrites the EXIF data unconditionally when saving an edited file, even if the image is not saved.
2) The code seems to assume that all of the images's EXIF data is stored in the mediadb record for the image. I don't think it is a good idea to do that, but even if it was, you don't have code to upgrade the database for existing images. So instead you need to read the existing EXIF data from the image, modifiy the orientation and write it back to the file. Please don't store all the EXIF data in the database. We don't have any use for it.
3) Github did not include your jpeg.js as a reviewable part of the patch. You must be treating it as a sub-repository or something. I don't know how those work, but I do feel like that module still needs review. I know I had lots of comments on it last time.
4) Generally speaking, jpeg.js seems too heavy (including an internal copy of BlobView.js and lots of unused strings) to be required by camera and by open.html. You've written a general-purpose EXIF reader/writer but that means that it is not as lean as I'd like for FirefoxOS. I can live with jpeg.js if you only load it when the user edits a photo, but I think it is too much to replace jpeg_metadata_parser.js
5) Your replacement for jpeg_metadata_parser.js does not seem to return the width and height of the image (or maybe I'm just not seeing the code for that) so I'm worried that things like get_image_size.js will break.
Attachment #8337151 -
Flags: review?(dflanagan) → review-
Comment 36•11 years ago
|
||
I just looked again at your jpeg.js file and realized that it doesn't just have a redundant internal copy of BlobView, it actually overwrites the existing BlobView object, replacing it with a class that has the same name but defines a different API.
I said above that I'd be okay with a heavy external library like this if you only load it when the user rotates an image. But obviously that won't work if it is overwriting globals. In addition to BlobView, it looks like jpeg.js defines 3 or 4 other globals. Can you modify it so that it defines only a single global object?
Comment 37•11 years ago
|
||
Going to pull needinfo for 1.3 evaluation now given the above review-
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(fabrice)
Assignee | ||
Comment 38•11 years ago
|
||
(In reply to David Flanagan [:djf] from comment #35)
> Comment on attachment 8337151 [details] [review]
> Pull Request
>
> r- because:
>
> 1) the code rewrites the EXIF data unconditionally when saving an edited
> file, even if the image is not saved.
I'm not sure I understood your comment. Did you mean "even if the metadata is not modified"? The patch only rewrites the EXIF metadata when the function saveEditedImage is invoked. We have to always save the EXIF even if it's not modified in order to copy over the metadata of the original image to the new image. When we call canvas.toBlob in getFullSizeBlob we lose the EXIF metadata of the copy. This address this bug:
Bug 798684 - [gallery] Editing a photo strips it of EXIF data
https://bugzilla.mozilla.org/show_bug.cgi?id=798684
>
> 2) The code seems to assume that all of the images's EXIF data is stored in
> the mediadb record for the image. I don't think it is a good idea to do
> that, but even if it was, you don't have code to upgrade the database for
> existing images. So instead you need to read the existing EXIF data from the
> image, modifiy the orientation and write it back to the file. Please don't
> store all the EXIF data in the database. We don't have any use for it.
Ok. I won't include the metadata in MediaDB
>
> 3) Github did not include your jpeg.js as a reviewable part of the patch.
> You must be treating it as a sub-repository or something. I don't know how
> those work, but I do feel like that module still needs review. I know I had
> lots of comments on it last time.
My jpeg.js is included in the PR but it's a new file so you don't see any diff. It lives in its own repo and it's just copied over. The dependency is not added as a submodule of gaia.
https://github.com/mozilla-b2g/exif-parser
It's the same pattern than the email app is using for mimeLib.js or pop.js or the calendar app for ical.js, caldav.js. I incorporated all your suggestions after your review of the last patch of the parser. Most of the strings are gone.
>
> 4) Generally speaking, jpeg.js seems too heavy (including an internal copy
> of BlobView.js and lots of unused strings) to be required by camera and by
> open.html. You've written a general-purpose EXIF reader/writer but that
> means that it is not as lean as I'd like for FirefoxOS. I can live with
> jpeg.js if you only load it when the user edits a photo, but I think it is
> too much to replace jpeg_metadata_parser.js
I'm going to take measurements but the difference between parsing 300 lines vs 3000 is a very small overhead in todays JS VMs. Most of the strings are gone after your reviews. I consider having code duplication is going to be a bigger problem in the long term. I can always keep optimizing the Exif parser if necessary. Let's get rid of jpeg_metadata_parser now and I'll keep working on the parser to make as lean as we need.
>
> 5) Your replacement for jpeg_metadata_parser.js does not seem to return the
> width and height of the image (or maybe I'm just not seeing the code for
> that) so I'm worried that things like get_image_size.js will break.
I created bugs to track the improvements in the parser:
Bug 947286 - [Exif Parser] Consolidate all global variables in a single one
https://bugzilla.mozilla.org/show_bug.cgi?id=947286
Bug 947305 - [Exif Parser] Make parser to read SOF3 JPEG segment
https://bugzilla.mozilla.org/show_bug.cgi?id=947305
Bug 947307 - [Exif Parser] Get rid of BlobView dependency
https://bugzilla.mozilla.org/show_bug.cgi?id=947307
Comment 39•11 years ago
|
||
This will ride 1.4 train. We will land this into master post 1.3 branch cut.
Thanks!
Hema
Target Milestone: 1.3 Sprint 4 - 11/8 → ---
Comment 40•11 years ago
|
||
not landing for 1.3, so removing the 1.3 milestone tag for this feature
blocking-b2g: - → 1.4?
Whiteboard: [ucid:Media28, 1.3:p2, ft:media], [MEDIA_TRIAGED] → [ucid:Media28, ft:media], [MEDIA_TRIAGED]
Updated•11 years ago
|
blocking-b2g: 1.4? → 1.5?
Comment 41•11 years ago
|
||
I will take up this Bug.
From the dmarcos patch I noticed that, We have 4 options, 0, 90, 180, 270.
Instead we can have two options rotate left and rotate right. Can we get the icons for the same?
Flags: needinfo?(dmarcos)
Flags: needinfo?(david)
Updated•11 years ago
|
Flags: needinfo?(david) → needinfo?(dflanagan)
Comment 42•11 years ago
|
||
(In reply to Madana Manjunatha from comment #41)
> I will take up this Bug.
>
> From the dmarcos patch I noticed that, We have 4 options, 0, 90, 180, 270.
> Instead we can have two options rotate left and rotate right. Can we get the
> icons for the same?
No. Our UX team has designed this feature to have four options, so that is the way it should be implemented. You can argue with UX if you want, but it seems silly to go through the design process again.
Note also that the EXIF writer code is going to land as part of bug 798684, so that part of Diego's patch will already be in place, and you'll only need to add the part that does the rotation and tweaks the rotation EXIF data.
Flags: needinfo?(dflanagan)
Updated•11 years ago
|
blocking-b2g: 1.5? → backlog
Whiteboard: [ucid:Media28, ft:media], [MEDIA_TRIAGED] → [ucid:Media28, ft:media], [MEDIA_TRIAGED] [priority]
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Updated•9 years ago
|
Flags: needinfo?(dmarcos)
Comment 43•7 years ago
|
||
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•