Return ISurfaceProvider objects from SurfaceCache lookup functions

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox50 wontfix, firefox51 wontfix)

Details

Attachments

(5 attachments)

Assignee

Description

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

3 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

3 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

3 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

3 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

Updated

3 years ago
Assignee: nobody → seth.bugzilla
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 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 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

3 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

3 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

3 years ago

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37340346a89e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1292290
Backed out for causing bug 1292290
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1dda3c8d9d2e7d7155f19edab3702bc84678819
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

3 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

3 years ago
backoutbugherder
Merged backout
https://hg.mozilla.org/mozilla-central/rev/c1dda3c8d9d2
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Assignee

Comment 17

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

Updated

3 years ago
See Also: → 1296147
Assignee

Updated

3 years ago
See Also: → 1296172
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)
(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.)
You need to log in before you can comment on or make changes to this bug.