917160 - preserveDrawingBuffer:false causes black canvas

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[James Darpinian]

Attachment: preserveDrawingBuffer.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.37 Safari/537.36

Steps to reproduce:

Open the attached file on Windows 7. It uses WebGL to draw a green canvas. It runs a continuous requestAnimationFrame loop, but only uses WebGL once every 60 frames.


Actual results:

The canvas flashes black. If you don't see it immediately, try switching tabs.


Expected results:

The canvas should be green continuously.

Comment 1

[Loic]

I'm not able to repro the issue with FF26 on Win 7.

Working range:
bad=2013-09-12
good=2013-09-13
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4e9c9c9dbf9&tochange=b9029b1de410

Comment 2

[Kelsey Gilbert [:jgilbert]]

Does not repro on Linux with basic or OGL layers.
I would guess it's one of the compositor changes that went in on that day. I'll switch over to windows to poke around.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Works on Win7 on 2013-09-09 and 2013-09-18.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Seems to work on Aurora26.

Does this still reproduce anywhere for you?

Comment 5

[James Darpinian]

Looks like it's fixed in Nightly 27. My version of Aurora is still 25 (where it still repros), how do I get Aurora 26?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Aurora 25 should update to 26 by itself. You can also grab it off our FTP servers: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/

Comment 7

[Kelsey Gilbert [:jgilbert]]

If it was reproducing on 25, that means it's in Beta, which is real bad.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Yep, that's what I see.

Comment 9

[James Darpinian]

I can reproduce it in 23, 24, and 25. It's been around for a long time apparently.

Actually, scratch my earlier report of it being fixed, because I just found a way to repro in 26 and 27 too. Start dragging the tab, and it will turn black as soon as the drag preview appears. This is a more reliable repro than before, though it can still happen randomly too.

Comment 10

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

(In reply to James Darpinian from comment #9)
> I can reproduce it in 23, 24, and 25. It's been around for a long time
> apparently.
> 
> Actually, scratch my earlier report of it being fixed, because I just found
> a way to repro in 26 and 27 too. 

Changed the tracking flags to match this comment.

Comment 11

[Kelsey Gilbert [:jgilbert]]

This reproduces consistently on 25 and below when you switch away from the testcase tab and switch back.
About 0.5s after switching back, the canvas almost always goes black for up to a second. During this time, we don't get to the SharedSurf part of UpdateSurface, likely because we don't think it's dirty.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Bas helped me figure this out. This is caused by (what appears to be) snap-shotting code, where we construct a BasicLayer tree. This means we call GLScreenbuffer::Readback() on the current consumer buffer, so that we can readback the pixels from what we're already displaying via d3d layers.

What this does on ANGLE is eglMakeCurrent() using the surface we desire. However, once we've swapped out the usual backbuffer for the current frontbuffer, we call ReadPixels, which sees that the 'draw' buffer is dirty, and so blits the cleared draw buffer onto our current frontbuffer!

The fix is to AssureBlitted when we change surfaces with MakeCurrent.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: blit-makecurrent

This should do it, but I'm waiting on a clobber build. :\

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: AssureBlitted before changing EGLSurfaces with MakeCurrent

qref'd

Comment 15

[James Darpinian]

Awesome! Any idea whether the fix can be backported to 25 and 26?

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to James Darpinian from comment #15)
> Awesome! Any idea whether the fix can be backported to 25 and 26?

26 yes, and probably 25.

Comment 17

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1134b18371d3

Comment 18

[Kelsey Gilbert [:jgilbert]]

Bug 924241 is somewhat related, where we forcibly clear-to-black when resizing the canvas.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1134b18371d3

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 813915 [details] [diff] [review]
patch: AssureBlitted before changing EGLSurfaces with MakeCurrent

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Snapshotting + StreamingBuffers
User impact if declined: Intermittent rendering artifacts (black screens) on WebGL apps, particularly after changing tabs. Currently blocks activation of MapsGL on windows.
Testing completed (on m-c, etc.): local, on m-c
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None

Comment 21

[Alex Keybl [:akeybl]]

We'd like to unblock MapsGL, so we should take this on ESR 24 and pre-release branches. Is the current patch OK to approve for ESR24?

Comment 22

[Kelsey Gilbert [:jgilbert]]

(In reply to Alex Keybl [:akeybl] from comment #21)
> We'd like to unblock MapsGL, so we should take this on ESR 24 and
> pre-release branches. Is the current patch OK to approve for ESR24?

Yes.

Comment 23

[James Darpinian]

If this lands in 24, will the user agent string allow us to tell an updated version of 24 from a older version? I doubt we would want to enable WebGL for 24 unless we could guarantee that older versions wouldn't be a problem.

Comment 24

[Alex Keybl [:akeybl]]

(In reply to James Darpinian from comment #23)
> If this lands in 24, will the user agent string allow us to tell an updated
> version of 24 from a older version? I doubt we would want to enable WebGL
> for 24 unless we could guarantee that older versions wouldn't be a problem.

No, it intentionally will not. That being said, we can land there anyway and this feature can optionally be enabled for ESR users once most Release channel users update away.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8320c2fdf240
https://hg.mozilla.org/releases/mozilla-beta/rev/7057e2497752
https://hg.mozilla.org/releases/mozilla-esr24/rev/77a6bd2d8c74

Comment 27

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0



The issue still reproduces, the canvas flashes black if the "alt" key is pressed to bring up the menu bar on Firefox 25 beta 7 (build ID: 20131010180222) and on latest Aurora (build ID: 20131010004002).

Comment 28

[James Darpinian]

Summoning the menu bar with 'alt' resizes the page, which is tracked separately in bug 924241. I've confirmed that bug 924241 doesn't affect Google Maps because we redraw the canvas every time the page is resized. This patch fixes the issue for us in 25, 26, and 27. Thanks for being so responsive on this!

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.1; rv:27.0) Gecko/20100101 Firefox/27.0

Also verified on latest Nightly and indeed it not reproduces if the menu bar is not summoned. 

Marking as verified for FF 25, 26 and 27.

Comment 30

[Matt Wobensmith [:mwobensmith][:matt:]]

Cornel, can you verify ESR 24? Thank you.

Comment 31

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Matt Wobensmith from comment #30)
> Cornel, can you verify ESR 24? Thank you.

Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0

Verified on ESR 24 (build ID: 20131021230807) and it not reproduces if the menu bar is not summoned (tracked in bug 924241). 
Marking this issue as verified.