Closed
Bug 1355898
Opened 7 years ago
Closed 7 years ago
WebGL game objects are not rendered correctly
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
VERIFIED
FIXED
mozilla55
People
(Reporter: phorea, Assigned: tnikkel)
References
()
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
4.10 KB,
patch
|
jgilbert
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
gchang
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
1.57 KB,
patch
|
jgilbert
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
gchang
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
[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•7 years 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•7 years ago
|
||
Assignee: nobody → tnikkel
Attachment #8857677 -
Flags: review?(jgilbert)
Comment 3•7 years ago
|
||
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•7 years 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•7 years ago
|
||
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 6•7 years ago
|
||
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•7 years ago
|
||
Attachment #8857765 -
Attachment is obsolete: true
Attachment #8857772 -
Flags: review?(jgilbert)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8857773 -
Flags: review?(jgilbert)
Comment 9•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8857773 -
Flags: review?(jgilbert) → review+
Comment 10•7 years 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
Comment 11•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bff28a19134f https://hg.mozilla.org/mozilla-central/rev/ba945b91cc6d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 13•7 years ago
|
||
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d09c1172a183 Add crashtest.
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d09c1172a183
Comment 15•7 years ago
|
||
Looks worth considering for uplift to 54, not so certain about ESR52, though. Please nominate for wherever you think makes the most sense :)
Assignee | ||
Comment 16•7 years 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•7 years ago
|
||
Comment on attachment 8857772 [details] [diff] [review] rename flag See comment 16
Attachment #8857772 -
Flags: approval-mozilla-aurora?
Comment 18•7 years ago
|
||
Hi :petruta, can you help check if the fix is expected in the latest nightly?
Flags: needinfo?(petruta.rasa)
Reporter | ||
Comment 19•7 years 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.
Comment 20•7 years ago
|
||
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•7 years ago
|
Attachment #8857772 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/076f4a79219b https://hg.mozilla.org/releases/mozilla-aurora/rev/3bab94261ee0
Comment 22•7 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92298693&repo=mozilla-aurora from aurora
Flags: needinfo?(tnikkel)
Comment 23•7 years ago
|
||
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?
Updated•7 years ago
|
Attachment #8857773 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 24•7 years 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 25•7 years ago
|
||
Comment on attachment 8857772 [details] [diff] [review] rename flag Beta54+.
Attachment #8857772 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8857773 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 26•7 years ago
|
||
(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•7 years ago
|
||
bugherder uplift |
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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(tnikkel)
Comment 28•7 years ago
|
||
Petruta, could you please verify this on 54b1 as well?
Flags: needinfo?(petruta.rasa)
Reporter | ||
Comment 29•7 years ago
|
||
Verified as fixed on Firefox 54 beta 1 across platforms.
Flags: needinfo?(petruta.rasa)
Comment 30•7 years ago
|
||
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•7 years 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•7 years 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 33•7 years ago
|
||
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•7 years ago
|
Attachment #8857773 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment hidden (obsolete) |
Comment 35•7 years 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
Updated•6 years ago
|
Flags: in-qa-testsuite+
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•