981005 - [B2G][Gallery][Crop Image]Image no longer displays after user presses 'undo' button.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ross Kuhlman ([e-mail])]

Attachment: 2014-03-07-09-44-51.png

When editing photos from the gallery, the user can crop images to predefined dimensions. If the user presses the 'undo' button, the cropping lines will revert to their original dimensions, but the image is no longer visible.

Repro Steps:
1) Updated Buri to BuildID: 20140307040203
2) Launch gallery app.
3) Tap on an image.
4) Tap on the 'edit' button in the toolbar.
5) Tap on the 'crop' button.
6) Tap on the 'Undo' button.

Actual:
The image is no longer displayed, but crop lines are visible.

Expected:
The image is still displayed.

Environmental Variables:
Device: Buri v1.4 Moz RIL
BuildID: 20140307040203
Gaia: 04eb7996543f114133d1367f834a4d88b68c60ac
Gecko: b0e7f63c2138
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Notes:

Repro frequency: 100%
Link to failed test case:
See attached: screenshot

Comment 1

[Jason Smith [:jsmith]]

Does this reproduce on 1.3?

Comment 2

[Fred Wenzel [:wenzel]]

I can confirm this on 1.4 (latest OTA that was delivered to my GP Keon).

Comment 3

[Harvey]

This issue does not reproduce on the latest 1.3

Environmental Variables:
Device: Buri v1.3 Mozilla
BuildID: 20140310004001
Gaia: 78c30d7ed6f6e30337d6c05453b867f5e97e42bc
Gecko: 00f249d54bda
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 4

[Hema Koka [:hema]]

Jim or Russ, can one of you please investigate this issue...

regression issue per comment 3, blocking

Comment 5

[Russ Nicoletti [:russn]]

I will investigate.

Comment 6

[Russ Nicoletti [:russn]]

The problem appears to be with ImageEditor.generateNewPreview

Continuing to investigate...

Comment 7

[Russ Nicoletti [:russn]]

Here's what I have found out:

First, while investigating the difference between 1.3 and 1.4 behavior, I did see the issue intermittently in 1.3.

Second, I have discovered there appears to be a timing issue in ImageEditor.prototype.finishEdit. When I introduce a delay before calling the callback, the image does not disappear.

For example, instead of https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/ImageEditor.js#L685-L687 

I used
  if (callback) {
    setTimeout(function() {callback()}, 2000);
  }

I'm not sure why the delay corrects the problem. Needs more investigation.

Comment 8

[Russ Nicoletti [:russn]]

More information on what appears to be a timing issue: 

If when entering the crop screen, there is a delay introduced in showCropOverlay -- for example, when setting a breakpoint in the debugger -- the undo-crop works properly, the image does not disappear.

When entering the crop screen without any delay, the image disappears when it is drawn by this line of code: https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/ImageEditor.js#L1551

Furthermore, when on the crop screen after having introduced a delay in showCropOverlay, if the delay is removed, the first time undoing a crop the image does not disappear but the crop overlay does not appear. The second time undoing a crop results in the image disappearing -- no image, no crop overlay.

The situation in reverse is similar. When introducing a delay after undo crop produces no image, the first time undoing also produces no image but with a crop overlay and the second time undoing a crop does produce an image and a crop overlay.

Comment 9

[Russ Nicoletti [:russn]]

It seems there is some interaction between ImageEditor.prototype.showCropOverlay and ImageEditor.prototype.draw, but I'm not sure what the nature of the interaction is.

Comment 10

[Chris Kreinbring [:CKreinbring]]

Regression windows:

TINDERBOX
No repro
Build ID: 20140306040204
Gecko: https://hg.mozilla.org/mozilla-central/rev/8122ffa9e1aa
Gaia: 9cb35e701df44766d9b3560b0defe0a401a0ecdd
Platform Version: 30.0a1
Firmware Version: V1.2-device.cfg

Repro
Build ID: 20140306041507
Gecko: https://hg.mozilla.org/mozilla-central/rev/4414ec1719aa
Gaia: 9cb35e701df44766d9b3560b0defe0a401a0ecdd
Platform Version: 30.0a1
Firmware Version: V1.2-device.cfg

Broken Gaia / Working Gecko - OK
Working Gaia / Broken Gecko - Fail
Gecko push log: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8122ffa9e1aa&tochange=4414ec1719aa


INBOUND
No repro
Build ID: 20140305134608
Gecko: https://hg.mozilla.org/integration/mozilla-inbound/rev/e91a61089c37
Gaia: 9cb35e701df44766d9b3560b0defe0a401a0ecdd
Platform Version: 30.0a1
Firmware Version: V1.2-device.cfg

Repro
Build ID: 20140305135105
Gecko: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f287c42495b
Gaia: 9cb35e701df44766d9b3560b0defe0a401a0ecdd
Platform Version: 30.0a1
Firmware Version: V1.2-device.cfg

Push log: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e91a61089c37&tochange=6f287c42495b

Comment 11

[Jason Smith [:jsmith]]

Three possible bugs in the regression range. They are all graphics related and all fixed by James.

James - Which bug do you think caused this?

Bugs in the Range:

* bug 980048
* bug 939276
* bug 956993

I'm resetting the assignee since the problem here is graphics related (not a front-end issue).

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Benoit is back from vacation first - can you take a look - George, can you follow up?  I want to make sure this isn't the shared context regression.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jason Smith [:jsmith] from comment #11)
> Three possible bugs in the regression range. They are all graphics related
> and all fixed by James.
> 
> James - Which bug do you think caused this?
> 
> Bugs in the Range:
> 
> * bug 980048
> * bug 939276
> * bug 956993
> 
> I'm resetting the assignee since the problem here is graphics related (not a
> front-end issue).

Not sure, but maybe bug 939276? None of these really changed what we do when drawing a SkiaGL canvas into a WebGL one, though.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

If this is a question of timing, and something that exists in 1.3 (with a lower frequency) then anything that changed the timing could have introduced the problem.  We'll probably need a direct fix, rather than looking for a problem with the bugs in the regression range, if this is the case.
CJ, do you have somebody to take a look?

Comment 15

[u459114]

Hi Jerry,
Please check this bug. <Look through comment 7 comment 8 comment 9 before dive into code :)>

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I can reproduce this problem on my device. I will check the crop function in gallery.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I just change the crop canvas to skia, then it works will.

https://github.com/mozilla-b2g/gaia/blob/f3c4a2fc0cfbeccc9c595f06b436707fa2e6ca0a/apps/gallery/js/ImageEditor.js#L825

-var context = this.cropContext = canvas.getContext('2d');
+var context = this.cropContext = canvas.getContext('2d', { willReadFrequently: true });

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I'm not familiar with the crop's code path in gallery.
I will test the comment 7,comment 8 and comment 9 to check if it is a timing issue.
Or is it the skiagl shared context issue?

James, do we have a pref to enable/disable skiagl shared context?

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jerry Shih[:jerry] from comment #18)
> 
> James, do we have a pref to enable/disable skiagl shared context?

Nope.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Does the code here draw one canvas into another one? If so, I have a hunch about what the problem could be. With the shared GLContext, we can now draw one canvas into another without any readback -- we just draw the backing texture directly. If that texture is somehow incomplete or unresolved, it could be bad, but AFAIK that shouldn't happen. Jeff do you know if that's possible? If everything is on one GLContext, presumably the GPU should order stuff appropriately?

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: possible fix

Can someone give this a shot? I don't have a b2g build environment handy right now.

Comment 22

[Russ Nicoletti [:russn]]

Attachment: image 1 of cropping with skia canvas

Comment 23

[Russ Nicoletti [:russn]]

Attachment: image 2 of cropping with skia canvas

Comment 24

[Russ Nicoletti [:russn]]

Attachment: image 1 of cropping with skiaGL canvas

Comment 25

[Russ Nicoletti [:russn]]

Attachment: image 2 of cropping with skiaGL canvas

Comment 26

[Russ Nicoletti [:russn]]

Regarding comment #17, I can confirm the undo crop problem -- image disappears -- is fixed by using "skia" canvas. However, the crop overlay is negatively affected. See attached images.

I will take a run at applying the patch from comment #21.

Comment 27

[George Wright (:gw280) (needinfo me!)]

I'm at GDC this week, hence the lack of communication from me on this bug. I think I should have some time tomorrow to look into this. snorp - if no one else tests your patch before tomorrow I definitely will.

Comment 28

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Russ Nicoletti [:russn] from comment #26)
> Regarding comment #17, I can confirm the undo crop problem -- image
> disappears -- is fixed by using "skia" canvas. However, the crop overlay is
> negatively affected. See attached images.
> 
> I will take a run at applying the patch from comment #21.

I have the different result. The crop overlay's line still works well. James, I will test your patch today(maybe 8 hr later).

Comment 29

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #21)
> Created attachment 8394334 [details] [diff] [review]
> possible fix
> 
> Can someone give this a shot? I don't have a b2g build environment handy
> right now.

With this patch, the crop mode works will with skiagl. The preview and the crop overlay still appear with undo button.

gaia:
var context = this.cropContext = canvas.getContext('2d');

Comment 30

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #20)
> Does the code here draw one canvas into another one? If so, I have a hunch
> about what the problem could be. With the shared GLContext, we can now draw
> one canvas into another without any readback -- we just draw the backing
> texture directly. If that texture is somehow incomplete or unresolved, it
> could be bad, but AFAIK that shouldn't happen. Jeff do you know if that's
> possible? If everything is on one GLContext, presumably the GPU should order
> stuff appropriately?

If everything's on the same context, and done from the same thread, it'll all be in-order. (excepting driver bugs, of course)

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, so I think maybe we're barking up the wrong tree here with the glFinish/Flush stuff. If that worked for folks (and it didn't for me on my own hamachi device) it was probably just coincidence. Investigating some more.

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can reproduce this on a desktop b2g nightly as well after enabling SkiaGL canvas there. It's not quite the same, though -- I end up with a corrupted image instead of a blank one.

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: screenshot of b2g desktop issue

Comment 34

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-981005-Ensure-the-correct-GLContext-is-current-b.patch

Looks like we never ensured our context was current before trying to update, and we got lucky 99% of the time.

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8398141 [details] [diff] [review]
0001-Bug-981005-Ensure-the-correct-GLContext-is-current-b.patch

Review of attachment 8398141 [details] [diff] [review]:
-----------------------------------------------------------------

Embarrassing.

Comment 36

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea0bf2b69f0

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ea0bf2b69f0

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/11f743514fbd