WebGL game objects are not rendered correctly

VERIFIED FIXED in Firefox 54

Status

()

Core
ImageLib
VERIFIED FIXED
2 months ago
11 days ago

People

(Reporter: petruta, Assigned: tnikkel, NeedInfo)

Tracking

({regression})

51 Branch
mozilla55
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 wontfix, firefox53 wontfix, firefox54 verified, firefox55 verified, firefox-esr45 unaffected, firefox-esr52 affected)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 months ago
[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
(Assignee)

Comment 1

2 months ago
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.
(Assignee)

Comment 2

2 months ago
Created attachment 8857677 [details] [diff] [review]
use first frame
Assignee: nobody → tnikkel
Attachment #8857677 - Flags: review?(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.
Attachment #8857677 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 4

2 months ago
(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
(Assignee)

Comment 5

2 months ago
Created attachment 8857765 [details] [diff] [review]
use first frame v2

Updated the comment to clarify. I'm open to suggestions for how to make the flag clearer.
Attachment #8857677 - Attachment is obsolete: true
Attachment #8857765 - Flags: review?(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!
Attachment #8857765 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 7

2 months ago
Created attachment 8857772 [details] [diff] [review]
rename flag
Attachment #8857765 - Attachment is obsolete: true
Attachment #8857772 - Flags: review?(jgilbert)
(Assignee)

Comment 8

2 months ago
Created attachment 8857773 [details] [diff] [review]
use first frame v3
Attachment #8857773 - Flags: review?(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.
Attachment #8857772 - Flags: review?(jgilbert) → review+
Attachment #8857773 - Flags: review?(jgilbert) → review+

Comment 10

2 months ago
Pushed by tnikkel@gmail.com:
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
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.
status-firefox53: affected → wontfix
https://hg.mozilla.org/mozilla-central/rev/bff28a19134f
https://hg.mozilla.org/mozilla-central/rev/ba945b91cc6d
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 13

2 months ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09c1172a183
Add crashtest.

Comment 14

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d09c1172a183
Looks worth considering for uplift to 54, not so certain about ESR52, though. Please nominate for wherever you think makes the most sense :)
status-firefox52: --- → wontfix
Flags: needinfo?(tnikkel)
Flags: in-testsuite+
(Assignee)

Comment 16

a month ago
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
Flags: needinfo?(tnikkel)
Attachment #8857773 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 17

a month ago
Comment on attachment 8857772 [details] [diff] [review]
rename flag

See comment 16
Attachment #8857772 - Flags: approval-mozilla-aurora?
Hi :petruta, 
can you help check if the fix is expected in the latest nightly?
Flags: needinfo?(petruta.rasa)
(Reporter)

Comment 19

a month ago
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(petruta.rasa)
Comment on attachment 8857773 [details] [diff] [review]
use first frame v3

Fix a WebGL issue and was verified. Aurora54+.
Attachment #8857773 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

a month ago
Attachment #8857772 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/076f4a79219b
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bab94261ee0
status-firefox54: affected → fixed
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92298693&repo=mozilla-aurora from aurora
status-firefox54: fixed → affected
Flags: needinfo?(tnikkel)
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 :)
Attachment #8857772 - Flags: approval-mozilla-beta?
Attachment #8857773 - Flags: approval-mozilla-beta?
(Assignee)

Comment 24

a month ago
(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.
Flags: needinfo?(cbook)
Comment on attachment 8857772 [details] [diff] [review]
rename flag

Beta54+.
Attachment #8857772 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

a month ago
Attachment #8857773 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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
Flags: needinfo?(cbook)

Comment 27

a month ago
bugherderuplift
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
status-firefox54: affected → fixed
(Assignee)

Updated

a month ago
Flags: needinfo?(tnikkel)
Petruta, could you please verify this on 54b1 as well?
Flags: needinfo?(petruta.rasa)
(Reporter)

Comment 29

a month ago
Verified as fixed on Firefox 54 beta 1 across platforms.
status-firefox54: fixed → verified
Flags: needinfo?(petruta.rasa)
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?
Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.