Closed
Bug 1289628
Opened 7 years ago
Closed 7 years ago
Return ISurfaceProvider objects from SurfaceCache lookup functions
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
WONTFIX
mozilla50
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox50 | --- | wontfix |
firefox51 | --- | wontfix |
People
(Reporter: seth, Assigned: seth)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
15.16 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
4.38 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.35 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
24.66 KB,
patch
|
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
At this point we want to start moving animation code into an ISurfaceProvider, but to do that we need to expose ISurfaceProvider directly to code that performs lookups in the SurfaceCache. There are two main reasons why we want to do this: (1) We don't want to composite animation synchronously inside a SurfaceCache method which holds the lock, as that could potentially block all other decoding for a long time. Compositing animations asynchronously (e.g. via the media framework) would remove this concern, but that's something that's not going to happen right away, so for now we have to address it. (2) In this short term, we'll also need to access the ISurfaceProvider for an animated image in order to advance the animation. (i.e., we need to access it in RasterImage::RequestRefresh()). This concern will probably go away sooner as FrameAnimator gets split up into multiple simpler classes, but for now this remains an issue. (How would we avoid this once FrameAnimator gets refactored, you ask? If we have the animation's "timeline" available separately from the decoded surfaces, we can just store the timeline on RasterImage and update it in RequestRefresh(), generating an invalidation if needed. When drawing we can then simply request the appropriate frame. This isn't as big a change as it sounds; I expect us to get there soon.)
Assignee | ||
Comment 1•7 years ago
|
||
This patch updates the SurfaceCache code itself. Note that we need to remove one additional feature from SurfaceCache to make this work: we can no longer automatically remove surfaces which had their volatile buffers freed by the OS. This is because to check, we'd have to call ISurfaceProvider::DrawableRef(), and doing that inside SurfaceCache code is exactly what we're trying to avoid. We could eventually move this check back into SurfaceCache once we only retrieve ISurfaceProviders for drawing purposes again, but I'm no longer sure that's actually the right layer at which to solve this problem, since we can't automatically handle device resets in SurfaceCache code. (They can cause a surface to become invalid asynchronously and there's no way to "lock" a surface to prevent them.) That means we'll always need some check outside of the SurfaceCache anyway. I suspect we're probably better off handling both cases in whatever class eventually replaces imgFrame once all this refactoring is done.
Attachment #8774950 -
Flags: review?(dholbert)
Assignee | ||
Comment 2•7 years ago
|
||
This updates the RasterImage::LookupFrame() code to handle receiving an ISurfaceProvider and checking that it can successfully get a DrawableFrameRef from it. Nothing else after that point has to change.
Attachment #8774951 -
Flags: review?(edwin)
Assignee | ||
Comment 3•7 years ago
|
||
This is a similar update to part 1b, but for VectorImage. To keep this clean, VectorImage really needed a method similar to RasterImage::LookupFrame(), so I've added VectorImage::LookupCachedSurface() in this patch.
Attachment #8774953 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•7 years ago
|
||
This patch updates FrameAnimator. Not too much to do here. In GetCompositedFrame() we need to wrap anything that doesn't come from the SurfaceCache in an ISurfaceProvider, and that in GetRawFrame() we need to add the additional check that we can get a DrawableFrameRef.
Attachment #8774954 -
Flags: review?(edwin)
Assignee | ||
Comment 5•7 years ago
|
||
Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d9686be42f7
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → seth.bugzilla
Attachment #8774951 -
Flags: review?(edwin) → review+
Comment on attachment 8774954 [details] [diff] [review] (Part 1d) - Update FrameAnimator to work with ISurfaceProviders. Review of attachment 8774954 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks for the patches Seth -- always good to see new volunteers contributing! ;-)
Attachment #8774954 -
Flags: review?(edwin) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8774953 [details] [diff] [review] (Part 1c) - Update VectorImage to work with ISurfaceProviders. Review of attachment 8774953 [details] [diff] [review]: ----------------------------------------------------------------- r=me on 1c
Attachment #8774953 -
Flags: review?(dholbert) → review+
Comment 8•7 years ago
|
||
Comment on attachment 8774950 [details] [diff] [review] (Part 1a) - Update SurfaceCache and LookupResult code to return ISurfaceProvider objects from Lookup*() functions. Review of attachment 8774950 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 1a, assuming you fold all of the "part 1*" patches together before landing (for more compile-friendly "hg bisect" experiences). Just a few observations: ::: image/SurfaceCache.cpp @@ +55,5 @@ > > // The single surface cache instance. > static StaticRefPtr<SurfaceCacheImpl> sInstance; > > + (Random new blank line here -- looks accidental, maybe? Or, fine to keep it, if it's intentional for better separation or whatever.) @@ -215,5 @@ > > private: > nsExpirationState mExpirationState; > RefPtr<ISurfaceProvider> mProvider; > - DrawableFrameRef mDrawableRef; (Oops, looks like this variable has been unused since bug 1282327 ( http://hg.mozilla.org/mozilla-central/rev/2984a22f9dcb#l3.74 Thanks for cleaning it up here!) @@ +637,2 @@ > > + RefPtr<ISurfaceProvider> provider = surface->Provider(); The "surface" naming here (& with other CachedSurface variables, and that class name itself) is a bit confusing/paradoxical now -- here, "surface" is paradoxically "providing" us with a SurfaceProvider. :) And that's quite backwards. Should we rename CachedSurface and its instances at some point? (Is that already happening in another bug / patch somewhere?) Any thoughts on a better name?
Attachment #8774950 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 9•7 years ago
|
||
Thanks for the reviews! (In reply to Edwin Flores [:eflores] [:edwin] from comment #6) > LGTM. Thanks for the patches Seth -- always good to see new volunteers > contributing! > > ;-) =p (In reply to Daniel Holbert [:dholbert] from comment #8) > The "surface" naming here (& with other CachedSurface variables, and that > class name itself) is a bit confusing/paradoxical now -- here, "surface" is > paradoxically "providing" us with a SurfaceProvider. :) And that's quite > backwards. > > Should we rename CachedSurface and its instances at some point? (Is that > already happening in another bug / patch somewhere?) Any thoughts on a > better name? Yeah, I agree. In the documentation we've moved in the direction of just saying "cache entry", which is a little generic, but it's an option. We could also replace "Surface" with "Provider" in some cases and see if that reads better. I haven't tried either approach out yet, but we should definitely find something more readable and change it while this code is getting touched.
Comment 10•7 years ago
|
||
Pushed by seth.bugzilla@blackhail.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/37340346a89e Return ISurfaceProvider objects from SurfaceCache lookup functions. r=dholbert,edwin
Assignee | ||
Updated•7 years ago
|
Blocks: streaming-gif
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/37340346a89e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 12•7 years ago
|
||
Backed out for causing bug 1292290 https://hg.mozilla.org/integration/mozilla-inbound/rev/c1dda3c8d9d2e7d7155f19edab3702bc84678819
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 13•7 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: backing out this bug [User impact if declined]: some images won't paint (aka bug 1292290) [Describe test coverage new/current, TreeHerder]: none [Risks and why]: just a back out, should be safe [String/UUID change made/needed]: none
Attachment #8781708 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 14•7 years ago
|
||
FWIW, I've prepared an additional patch for this part that should fix bug 1292290 (testing locally now). Since it's an Android-only problem and I don't have an Android device, I'm hoping people in bug 1292290 can help verify the fix. After the fix is verified, we can reland.
Great, thanks. We'd want it to ride 51, rather than uplift the new fix, but it'd be great to have it.
Comment 16•7 years ago
|
||
backout bugherder |
Merged backout https://hg.mozilla.org/mozilla-central/rev/c1dda3c8d9d2
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Assignee | ||
Comment 17•7 years ago
|
||
I ended up going in a different direction after some more contemplation. We should probably just mark this bug WONTFIX, since the new approach is quite dissimilar.
Resolution: FIXED → WONTFIX
Updated•7 years ago
|
status-firefox49:
--- → unaffected
Comment on attachment 8781708 [details] [diff] [review] backed_out_changeset_37340346a89e for aurora Backing out a risky fix, Aurora50+
Attachment #8781708 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Wes, not sure how to set the status flag for Fx50 when we are backing out a fix. NI is just as an fyi to you. Thanks!
Flags: needinfo?(wkocher)
Comment 20•7 years ago
|
||
(Ultimately we'll want to set the firefox-50 status flag to "wontfix", just like for 51, since this was wontfix'ed after it was backed out [i.e. it's going to stay permanently backed out], and we won't ship this particular bug's patches in any release, per comment 17.)
https://hg.mozilla.org/releases/mozilla-aurora/rev/2a0981531355
Flags: needinfo?(wkocher)
You need to log in
before you can comment on or make changes to this bug.
Description
•