1289628 - Return ISurfaceProvider objects from SurfaceCache lookup functions

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1a) - Update SurfaceCache and LookupResult code to return ISurfaceProvider objects from Lookup*() functions.

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1b) - Update RasterImage::LookupFrame() to work with ISurfaceProviders.

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1c) - Update VectorImage to work with ISurfaceProviders.

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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1d) - Update FrameAnimator to work with ISurfaceProviders.

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d9686be42f7

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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!

;-)

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Daniel Holbert [:dholbert]]

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?

Comment 9

[Seth Fowler [:seth] [:s2h]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37340346a89e
Return ISurfaceProvider objects from SurfaceCache lookup functions. r=dholbert,edwin

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37340346a89e

Comment 12

[Timothy Nikkel (:tnikkel)]

Backed out for causing bug 1292290
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1dda3c8d9d2e7d7155f19edab3702bc84678819

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: backed_out_changeset_37340346a89e for aurora

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

Comment 14

[Seth Fowler [:seth] [:s2h]]

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.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout
https://hg.mozilla.org/mozilla-central/rev/c1dda3c8d9d2

Comment 17

[Seth Fowler [:seth] [:s2h]]

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.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8781708 [details] [diff] [review]
backed_out_changeset_37340346a89e for aurora

Backing out a risky fix, Aurora50+

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 20

[Daniel Holbert [:dholbert]]

(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.)

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a0981531355