1355898 - WebGL game objects are not rendered correctly

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Petruta Horea [:phorea], Desktop QA]

[Affected versions]:
- Firefox 52.0
- Firefox 53.0 RC
- Firefox 52.1.0esr
- latest Aurora 54.0a2
- latest Nightly 55.0a1

[Affected platforms]:
- Win 10 64-bit
- Ubuntu 14.04 64-bit
- Mac OS X 10.11

[Steps to reproduce]:
1. Open http://www.wordsaretoys.com/apps/easy/ game
2. Follow instructions to collect items
3. Move around to find "coin" and "wood"

[Expected result]:
- Items to find are correctly displayed

[Actual result]:
- Win + Ubuntu: Black rectangles are shown instead of actual images
- OSX: The items are missing (not shown at all), but the total number is increased when passing through areas where they could be found

Browser console error:
Error: WebGL: drawArrays: Active texture 0 for target 0x0de1 is 'incomplete', and will be rendered as RGBA(0,0,0,1), as per the GLES 2.0.24 $3.8.2: The dimensions of `level_base` are not all positive.

[Regression range]:
- Mozregression returned bug 1293472 as the culprit:
Found commit message:
Bug 1293472 (Part 4) - Test that single-frame and animated decodes can coexist for the same image. r=edwin

[Note]:
- "oil" is displayed correctly

Comment 1

[Timothy Nikkel (:tnikkel)]

We load this image

http://www.wordsaretoys.com/apps/easy/res/chest.gif

and decide that it is animated here

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/image/decoders/nsGIFDecoder2.cpp#701

because we read a delay time of 100 ms. The gif actually only has 1 frame, so it's not really animated. Then when the texImage2d call uses SurfaceFromElement to get the image we hit this

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/image/RasterImage.cpp#292

presumably because of the no premultiply alpha flag. That's probably why the image doesn't draw correctly too.

This used to work because bug 1293472 changed from asking for a specific frame number (this had to always be the first because there was only one frame) to asking for either static or animated.

According to bug 666855 texImage2d should be asking for the first frame. But it's not. So we should probably just fix that.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: use first frame

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8857677 [details] [diff] [review]
use first frame

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +296,5 @@
>  {
> +    // The canvas spec says that drawImage should draw the first frame of
> +    // animated images. The webgl spec doesn't mention the issue, so we do the
> +    // same as drawImage.
> +    uint32_t flags = nsLayoutUtils::SFE_WANT_FIRST_FRAME |

I think this would prevent us from getting subsequent frames from a video.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> I think this would prevent us from getting subsequent frames from a video.

SurfaceFromElement for video elements completely ignores the passed in flags

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/layout/base/nsLayoutUtils.cpp#7388

The comment for the SFE_WANT_FIRST_FRAME flag gives a short mention that the flag only applies to images.

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/layout/base/nsLayoutUtils.h#2080

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: use first frame v2

Updated the comment to clarify. I'm open to suggestions for how to make the flag clearer.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8857765 [details] [diff] [review]
use first frame v2

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

s/SFE_WANT_FIRST_FRAME/SFE_WANT_FIRST_FRAME_IF_IMAGE/g, please!

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: rename flag

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: use first frame v3

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8857772 [details] [diff] [review]
rename flag

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

Sweet, thanks for doing this clarification.

::: layout/base/nsLayoutUtils.h
@@ +2077,5 @@
>    enum {
>      /* When creating a new surface, create an image surface */
>      SFE_WANT_IMAGE_SURFACE = 1 << 0,
>      /* Whether to extract the first frame (as opposed to the
>         current frame) in the case that the element is an image. */

Huh, it already says it in the comment! Well it's much better with this name though.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bff28a19134f
Rename flag SFE_WANT_FIRST_FRAME to SFE_WANT_FIRST_FRAME_IF_IMAGE to represent what it does better. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba945b91cc6d
Use the first frame of animated images for texImage2d. r=jgilbert

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Not a new regression, I'm not going to consider this a release blocker for 53. We could still likely take a (verified) patch for 54.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/bff28a19134f
https://hg.mozilla.org/mozilla-central/rev/ba945b91cc6d

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09c1172a183
Add crashtest.

Comment 14

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/d09c1172a183

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Looks worth considering for uplift to 54, not so certain about ESR52, though. Please nominate for wherever you think makes the most sense :)

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857773 [details] [diff] [review]
use first frame v3

This request is for all three changesets landed from this bug.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1293472
[User impact if declined]: webgl that uses animated images as textures will fail to draw the images/textures
[Is this code covered by automated tests?]: added a new test for it in this bug
[Has the fix been verified in Nightly?]: I checked that it works for me, but not by QA
[Needs manual test from QE? If yes, steps to reproduce]: comment 0 has STR
[List of other uplifts needed for the feature/fix]: the three changesets that landed in this
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's what we used to do, we'll just draw black if we don't take this
[String changes made/needed]: none

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857772 [details] [diff] [review]
rename flag

See comment 16

Comment 18

[Gerry Chang [:gchang]]

Hi :petruta, 
can you help check if the fix is expected in the latest nightly?

Comment 19

[Petruta Horea [:phorea], Desktop QA]

The objects are now correctly rendered in 55.0a1 Nightly 2017-04-17 under Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.11.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8857773 [details] [diff] [review]
use first frame v3

Fix a WebGL issue and was verified. Aurora54+.

Comment 21

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/076f4a79219b
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bab94261ee0

Comment 22

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92298693&repo=mozilla-aurora from aurora

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8857772 [details] [diff] [review]
rename flag

The uplift was today, so this'll need Beta approval to get to 54 now. RelMan, please clear the Aurora approvals when approving for Beta :)

Comment 24

[Timothy Nikkel (:tnikkel)]

(In reply to Carsten Book [:Tomcat] from comment #22)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=92298693&repo=mozilla-
> aurora from aurora

That's because what was landed was the attached patches to this bug. We should be uplifting the actual changesets that land on mozilla-central, not attached patches. Very often there are differences between the two. The uplift also missed the test that I landed because I didn't attach a patch for it to any bug.

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8857772 [details] [diff] [review]
rename flag

Beta54+.

Comment 26

[Carsten Book [:Tomcat]]

(In reply to Timothy Nikkel (:tnikkel) from comment #24)
> (In reply to Carsten Book [:Tomcat] from comment #22)
> > backed out for bustage like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=92298693&repo=mozilla-
> > aurora from aurora
> 
> That's because what was landed was the attached patches to this bug. We
> should be uplifting the actual changesets that land on mozilla-central, not
> attached patches. Very often there are differences between the two. The
> uplift also missed the test that I landed because I didn't attach a patch
> for it to any bug.

Hi Timothy,

yes sorry for this, seems we sheriffs did a mistake here and appologize for this. 
Will also remind the team sorry again, will take care of the uplift to beta.

Cheers,
- Tomcat

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3c1bf407a189
https://hg.mozilla.org/releases/mozilla-beta/rev/2763894710d8
https://hg.mozilla.org/releases/mozilla-beta/rev/9edc76120a27

Comment 28

[Andrei Vaida [:avaida]]

Petruta, could you please verify this on 54b1 as well?

Comment 29

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed on Firefox 54 beta 1 across platforms.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

I've verified that this grafts cleanly to ESR52. Is this severe enough that we should consider backporting it there too or can we leave it?

Comment 31

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857772 [details] [diff] [review]
rename flag

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: low risk fix for rendering fix
User impact if declined: fail to draw some unusual images in webgl
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): low risk, only applies to animated images used as textures (very rare I'm assuming) which probably didn't render properly before this change anyway
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 32

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857773 [details] [diff] [review]
use first frame v3

[Approval Request Comment]
same as comment 31

Comment 33

[Gerry Chang [:gchang]]

Comment on attachment 8857772 [details] [diff] [review]
rename flag

Fix a WebGL issue. Let's uplift this to ESR52.3.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a505776470f3
https://hg.mozilla.org/releases/mozilla-esr52/rev/46c09c592d8a
https://hg.mozilla.org/releases/mozilla-esr52/rev/a09f9a680f64

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Backed out from ESR52 for Linux32 assertions in image/test/crashtests/delayedframe.sjs. Per IRC discussion with Timothy, it's possible that there's a real issue lurking and that fixing it on ESR52 will require more invasive changes than are warranted for this bug. Therefore, setting ESR52 to wontfix as well.
https://hg.mozilla.org/releases/mozilla-esr52/rev/e21dc2356baa

https://treeherder.mozilla.org/logviewer.html#?job_id=113075676&repo=mozilla-esr52

Comment 36

[Intermittent Failures Robot]

1 failures in 3601 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 1

Platform breakdown:

• linux1804-64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1355898&startday=2020-08-10&endday=2020-08-16&tree=all