891030 - [Gallery] [User Story] Edit image orientation to correct for tilt sensor inaccuracies

Status: RESOLVED WONTFIX

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

Description

[Sri Kasetti]

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

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

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

[Mike Habicher [:mikeh] (high bugzilla latency)]

Wikipedia also has a good overview of EXIF and issues related to it:
  http://en.wikipedia.org/wiki/Exif

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

dhylands, does DeviceStorage support in-place editing of files? Or does a single-byte change require a read-modify-write cycle?

Comment 4

[Dave Hylands [:dhylands]]

(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.

Comment 5

[Rob MacDonald [:robmac]]

Attachment: draft - gallery image rotation UI spec

Draft UI spec for discussion.

Comment 6

[David Flanagan [:djf]]

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.

Comment 8

[Diego Marcos [:dmarcos]]

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

Comment 9

[Diego Marcos [:dmarcos]]

Patryk. We need a new asset for the rotation button

Comment 10

[Patryk Adamczyk [:patryk] UX]

Peter, can you please provide Diego with the new action icon? Thanks.

Comment 11

[Peter La]

Attachment: Image Rotation Icon (3 Sizes)

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)

Comment 12

[Stephany Wilkes]

Rob will address the button, but I'm just noting that this issue should not be blocking.

Comment 13

[Rob MacDonald [:robmac]]

(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

Comment 14

[Diego Marcos [:dmarcos]]

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.

Comment 15

[Diego Marcos [:dmarcos]]

Attachment: Image edition in Android gallery

Comment 16

[Rob MacDonald [:robmac]]

(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

Comment 17

[Hema Koka [:hema]]

Currently being worked on for 1.2 release

Comment 18

[Hema Koka [:hema]]

Diego Marcos added a comment in Pivotal Tracker:   
   
Waiting for dependecies resolution before landing a patch for this

Comment 19

[Diego Marcos [:dmarcos]]

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?

Comment 20

[Rob MacDonald [:robmac]]

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

Comment 21

[Jesse]

It's urgent for us in v1.2 branch. Could you finish it as soon as possible. Thanks.

Comment 22

[Jason Smith [:jsmith]]

(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-

Comment 23

[Peter La]

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.

Comment 24

[Peter La]

Hi, I've posted the icon assets for Gallery Edit Mode to this bug https://bugzilla.mozilla.org/show_bug.cgi?id=933066

Comment 25

[Diego Marcos [:dmarcos]]

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?

Comment 26

[Peter La]

Hey Diego,

According to Patryk we are no longer supporting 2x resolution icons.

Comment 27

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

Comment 28

[Jean Gong :jgong]

Update whiteboard tag to follow format [ucid:{id}, {release}:p{1,2}, ft:{team-id}]

Comment 29

[Jean Gong :jgong]

Update whiteboard tag to follow format [ucid:{id}, {release}:p{1,2}, ft:{team-id}]

Comment 30

[Hema Koka [:hema]]

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.

Comment 31

[Jason Smith [:jsmith]]

(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

[Hema Koka [:hema]]

(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

[Jason Smith [:jsmith]]

Okay. Marcia - Can you look into this?

Comment 34

[Jean Gong :jgong]

What is the status on this for 1.3? Thank you.

Comment 35

[David Flanagan [:djf]]

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.

Comment 36

[David Flanagan [:djf]]

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

[Jason Smith [:jsmith]]

Going to pull needinfo for 1.3 evaluation now given the above review-

Comment 38

[Diego Marcos [:dmarcos]]

(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

[Hema Koka [:hema]]

This will ride 1.4 train. We will land this into master post 1.3 branch cut.

Thanks!
Hema

Comment 40

[Hema Koka [:hema]]

not landing for 1.3, so removing the 1.3 milestone tag for this feature

Comment 41

[Madana Manjunatha]

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?

Comment 42

[David Flanagan [:djf]]

(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.

Comment 43

[BMO Automation]

Firefox OS is not being worked on