1102712 - [Midori 2.0][Gallery] Edit mode displays low-res image on high-res devices with devicePixelRatio > 1

Status: RESOLVED FIXED

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

Description

[sync-1]

FireFox OS V2.0
 
 mozilla build id:20141019000201
 
 DEFECT DESCRIPTION:
 Image pixel falling after clicking edit picture
 
 REPRODUCING PROCEDURES:
 1.Lunch Camera，take one photo
 2.Click home to return idle,click gallery
 3.Click edit picture，image pixel falling-->K.O
 
 
 EXPECTED BEHAVIOUR:
 K.O-->Image pixel can not falling after clicking edit picture

Comment 1

[sync-1]

Attachment: log-840404

Comment 2

[sync-1]

Attachment: pic-840404

Comment 3

[dingp]

Please help to fix this bug, thanks!

Comment 5

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This appears to be happening on the flame device as well. Branch checks can be found on bug 1107179

Comment 6

[Wesly Huang (EPM)]

[Blocking Requested - why for this release]:

all branches are affected, looks worth to tag, nom. to 2.0 Triage for discussion.

also qawanted to see if this is seen in 1.3 so actually a regression.

Comment 7

[Wesly Huang (EPM)]

[Blocking Requested - why for this release]:

[Triage] Nom to 2.1 instead of tagging in 2.0 due to its timing. Also consider it as a less use case but good to improve in future releases.

Comment 8

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Wesly Huang from comment #6)
> also qawanted to see if this is seen in 1.3 so actually a regression.

I assume you meant to check 1.4 but I checked 1.3 and 1.4 anyway.

This issue occurs on Flame base image v165 (v1.4) and Flame base image v123 (v1.3). Edit mode shows a low resolution image.

There's not a tracking flag for v1.3 so I flipped v1.4 only.

Comment 9

[bhavana bajaj [:bajaj]]

NI Hema here to see when her team can prioritize it. Hema feel free to remove the blocking flag once you have found a right bucket for this issue.

Comment 10

[Hema Koka [:hema]]

Happening since 1.3 (perhaps even before). Too late for 2.1 in any case. Moving to 2.2 to discuss in triage. (please note, most folks in the media team are off from 22nd to jan 2nd due to holidays)

NI David to see if we can fix this in 2.2 timeframe.

Comment 11

[David Flanagan [:djf]]

I'm surprised this hasn't been reported before.  The image editing code was written back when all of our phones were 320x480, and it never got updated to handle phones where devicePixelRatio was > 1.0

This should be easy to fix, though it will probably have an impact on the performance of the image slider since there will be a lot more pixels to process in real time. There may also potentially be problems with using webgl on a larger image, though since the image preview will still be smaller than the screen it should be okay.

Comment 12

[David Flanagan [:djf]]

Taking this, and editing the title so I can understand it.

Comment 0 does not actually explain what the bug is, and attached screenshot is annotated in Chinese so it was not until comment #8 from Pi Wei that I actually understood what the issue was here.  (Thanks Pi Wei!)

Comment 13

[David Flanagan [:djf]]

Attachment: link to patch on github

Punam,

Would you review this patch when you return from vacation, please?

This is a long-standing bug that dates back at least to summer 2013 when Tom was working on the image editor. The issue is that that canvas we allocate to display the image preview is sized in CSS pixels rather than device pixels. On devices like flame the preview image is lower quality than it should be.

Fixing the size of the preview image alone is simply a matter of adding two lines, and with webgl editing, it still typically takes < 1ms to edit the preview when the user drags the exposure compensation slider, so performance is still very fast.

The trickier part of this patch is that the crop overlay canvas is also in CSS pixels and when we change the preview canvas to be sized in device pixels there are a variety of adjustments we need to make to convert device pixel coordinates back to CSS coordinates.

When testing this patch, please also try cropping with the pick activity (when attaching an image to a MMS message, for example) and be sure to also test what happens when you change the orientation of the phone while cropping. It all seems to work to me, but I'd like someone else to try it out too before I'm confident that I didn't forget anything.

Comment 14

[Punam Dahiya [:pdahiya]]

Hi David

I tested attached patch and in one of edge case I am seeing below error in logs

12-29 06:39:41.050 W/Gallery ( 2016): [JavaScript Error: "IndexSizeError: Index or size is negative or greater than the allowed amount" {file: "app://gallery.gaiamobile.org/js/ImageEditor.js" line: 809}]

Clicking on Apply in crop Edit Mode fails and throws this error. As this error is intermittent, I will update bug with more precise steps to replicate. Thanks

Comment 15

[Punam Dahiya [:pdahiya]]

Attachment: Sample Image to replicate IndexSizeError

Comment 16

[Punam Dahiya [:pdahiya]]

Steps to replicate IndexSizeError in #comment 15

1. Open attached image and click on edit image icon.
2. Click crop and select 3:2 crop options and hit Apply
3. Click crop  and select back to original (leftmost icon) and hit cancel x
4. Click crop and select back to original (leftmost icon) and hit Apply

Apply fails with the IndexSizeError in logs.

Comment 17

[Punam Dahiya [:pdahiya]]

On debugging Line 809 https://github.com/mozilla-b2g/gaia/pull/26921/files#diff-6e48fa715fc315697388e7075d015a41R809

this.source.height (1513) is more than this.original.height (1512) causing context.drawImage to fail.

this.source.height is computed inside cropImage https://github.com/davidflanagan/gaia/blob/bug1102712/apps/gallery/js/ImageEditor.js#L1541


Preview Crop Region fraction bottom variable value is 1.00112 on L1523 causing Math.floor(bottom * this.source.height) to be computed to 1513.

Comment 18

[Punam Dahiya [:pdahiya]]

Comment on attachment 8539481 [details] [review]
link to patch on github

Hi David
The patch works for cropping pick activity and for resize when changing orienation while cropping. It's r- only because of IndexSizeError which I think should be handled while computing preview crop region fraction values. Thanks!

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8539481 [details] [review]
link to patch on github

Thanks for catching that, Punam. Having to deal with dpr allows all kinds of rounding and off-by-one issues to creep in. I think the ultimate cause of the bug was at lines 1132 and 1133 where I was using ceil() instead of floor() when converting from device pixels to css pixels.  I've added a new commit that always uses ceil() when multiplying by dpr and uses floor() when dividing by dpr. To be safe, though, I've also added Math.min() to be sure that we never get ratios > 1 in cropImage().

I also had to change hasBeenCropped(). I found that if I multiplied the CSS pixels by dpr and used ceil() it would sometimes return true even when the image had not been cropped. But if I divided the device pixels by dpr and used floor() then I did not not get those false positives. Hopefully there are not any more of these kinds of things, but I'd appreciate another careful round of testing on this.

Comment 20

[David Flanagan [:djf]]

Also, I intentionally left the new commit unsquashed so you could easily see what I changed here.

Comment 21

[Punam Dahiya [:pdahiya]]

Comment on attachment 8539481 [details] [review]
link to patch on github

Thanks David, I have tested and updated patch looks good and has r+.

Comment 22

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/e77a532025e13605dcd208dc0be8cd97fea5b19a