Shouldn't use WANT_NEW_SURFACE when uploading DOM elements to WebGL textures

RESOLVED FIXED in mozilla20

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

(Blocks: 1 bug)

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 693043 [details] [diff] [review]
Don't WANT_NEW_SURFACE

Thanks to Jeff M for spotting this; we seem to have been doing this since immemorial times, so CC'ing Vlad to check if there was any reason for that?

https://tbpl.mozilla.org/?tree=Try&rev=d3d5a5c7b9af
Attachment #693043 - Flags: review?(jgilbert)
Comment on attachment 693043 [details] [diff] [review]
Don't WANT_NEW_SURFACE

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

This looks like it'll be fine, since when we do convert, we convert into a new buffer. I would prefer this was made explicit by keeping track of this as a `const gfxImageSurface*`, so that we know we shouldn't mess with it.
Attachment #693043 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 2

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/852237e60012

Since there is no gfxImageSurface * directly around here, that sounds like a nontrivial change, you're welcome to do it if you care :)
Assignee: nobody → bjacob
Target Milestone: --- → mozilla20
(Assignee)

Comment 3

5 years ago
Having some second thoughts on this. Why would somebody _ever_ want to do SFE_WANT_NEW_SURFACE? Makes me afraid I'm missing something here (my irrational fear, not understanding this well, is that in the case of DOM elements whose image data can change with time, such as animated images, the SFE_WANT_NEW_SURFACE might actually have been needed).
(In reply to Benoit Jacob [:bjacob] from comment #3)
> Having some second thoughts on this. Why would somebody _ever_ want to do
> SFE_WANT_NEW_SURFACE? Makes me afraid I'm missing something here (my
> irrational fear, not understanding this well, is that in the case of DOM
> elements whose image data can change with time, such as animated images, the
> SFE_WANT_NEW_SURFACE might actually have been needed).

I don't think we would change the contents of a surface for an animated image while script is running. So I don't think that would be a problem for WebGL where I think you're going to use the surface contents immediately without returning to the event loop.

The other users of SFE_WANT_NEW_SURFACE:
-- cloning an nsHTMLMediaElement, e.g. for printing. There I think we probably do want a new surface.
-- creating a pattern from an image. I'm not sure about that one. Since we ask for the first frame, we certainly shouldn't get a surface that changes due to animation.

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/852237e60012
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

5 years ago
Comment on attachment 693043 [details] [diff] [review]
Don't WANT_NEW_SURFACE

[Approval Request Comment]
Bug caused by (feature/regressing bug #): always had this bug
User impact if declined: unnecessary temporary memory usage during WebGL texture loading; this matters on low-spec mobile devices (e.g. B2G) where it can make the difference between crash and not crash.
Testing completed: landed yesterday
Risk to taking this patch (and alternatives if risky): not risky from above discussion
String or UUID changes made by this patch: none
Attachment #693043 - Flags: approval-mozilla-b2g18?
Attachment #693043 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Blocks: 820217

Comment 7

5 years ago
Comment on attachment 693043 [details] [diff] [review]
Don't WANT_NEW_SURFACE

B2G WebGL perf has not been a high priority, and we're not sure of the final user impact here. Let's be risk averse.
Attachment #693043 - Flags: approval-mozilla-b2g18?
Attachment #693043 - Flags: approval-mozilla-b2g18-
Attachment #693043 - Flags: approval-mozilla-aurora?
Attachment #693043 - Flags: approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.