Note: There are a few cases of duplicates in user autocompletion which are being worked on.

WebGL game objects are not rendered correctly

VERIFIED FIXED in Firefox 54

Status

()

Core
ImageLib
VERIFIED FIXED
3 months ago
10 days ago

People

(Reporter: petruta, Assigned: tnikkel)

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 wontfix)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 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

3 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

3 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

3 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

3 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

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

Comment 8

3 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

3 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

Comment 12

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bff28a19134f
https://hg.mozilla.org/mozilla-central/rev/ba945b91cc6d
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 13

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

Comment 14

3 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

3 months 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

3 months 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

3 months 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

3 months ago
Attachment #8857772 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 21

3 months ago
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

3 months 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

3 months 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

3 months 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

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

Comment 29

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

Comment 31

29 days ago
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.
Flags: needinfo?(tnikkel)
Attachment #8857772 - Flags: approval-mozilla-esr52?
(Assignee)

Comment 32

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

[Approval Request Comment]
same as comment 31
Attachment #8857773 - Flags: approval-mozilla-esr52?
Comment on attachment 8857772 [details] [diff] [review]
rename flag

Fix a WebGL issue. Let's uplift this to ESR52.3.
Attachment #8857772 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Updated

11 days ago
Attachment #8857773 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment hidden (obsolete)

Comment 35

10 days ago
backout
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
status-firefox-esr52: fixed → wontfix
You need to log in before you can comment on or make changes to this bug.