773469 - OMTC: Remove unnecessary layer transactions when using async-video

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Async-video (see bug 598868) allows ImageContainers to forward frames directly to the compositor thread/process, without using the main thread. The ImageBridge protocol only needs one transaction at the beginning in order to pass to the ShadowImageLayer and ID corresponding to the image stream.

Currently, while playing a video we keep triggering layer transactions continuously, which is not useful since the video frames are not forwarded through these transactions.

We should remove these layer transactions to reduce unnecessary main-thread activity.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Don't always call Invalidate when using async-video

I started working on this and then switched back to working on bug 580784.

For this bug I tried several things:
First I tried to have VideoFrameContainer::RenderVideoFrame return true if it needs invalidation and false otherwise, and pass this argument along with the tasks that are dispatched to the main thread (and that will eventually call invalidate), mostly adding a parameter to UpdatePlaybackPosition and nsBuiltinDecoder::PlaybackPositionChanged, but this is somewhat awkward because PlaybackPositionChanged is overloaded by the different decoder implementations, and it did not really make sens that they have to care about "bool shoudlInvalidate".
I also tried a few other things involving the state machine to handle decicing when to invalidate, but it was not very good because it make more sense that the VideoFrameContainer handles that.

I ended up simply storing a boolean in the VideoFrameContainer telling whether or not we should invalidate when Invalidate is invoked. It is initialized to true and then flipped to false after the first call to Invalidate.

The basic idea is that we still want to invalidate sometimes, because the layer will not be created if we don't invalidate at least once. There will probably be other cases where we want to invalidate, I'm not really sure. So this version of the patch simply invalidates only once, which is enough to build the layer after what the ImageBridge protocol takes care of triggering compositing.

this is not a final solution, i barely tested it, but it looks like the simplest way to solve the problem even though it's likely that we will need something a bit more complex to decide when to invalidate when we use async-video.

It would probably be a good thing to rename VideoFrameContainer::Invalidate() into VideoFrameContainer::MaybeInvalidate() to make it clearer that the VideoFrameContainer actually decides whether or not we need to invalidate.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

this looks like the right approach.

Comment 3

[Nicolas Silva [:nical]]

I am looking for cases where we need to rebuild the layers without rebuilding the nsHTMLMediaElement (in chich case I need to make sure we Invalidate at least once to pass the image container ID to the ShadowImageLayer through a layer transaction). Any ideas?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The nsHTMLMediaElement's ImageContainer never changes so you should be fine as long as there's a layer transaction to set things up the first time.

BTW you should implement ScaleMode SCALE_PRESERVE_ASPECT_RATIO_CONTAIN (see ImageLayers.h) to ensure that if the image size or intrinsic size change asynchronously, we keep scaling the image to fit the video frame in the right way. I'm not sure how to construct a testcase where the video size changes halfway through, though.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And you might want to refer to bug 754542, where we discussed how ImageLayer's scaling API should evolve.

Comment 6

[Nicolas Silva [:nical]]

Attachment: Don't always call Invalidate when using async-video

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 650556 [details] [diff] [review]
Don't always call Invalidate when using async-video

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

::: content/media/VideoFrameContainer.cpp
@@ +51,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Must call on main thread");
> +
> +  if (!mNeedInvalidation) {
> +    return;
> +  } else if (mImageContainer && mImageContainer->IsAsync()) {

Don't put else after a return. Just start the new if as the next statement.

Comment 8

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75023a9d0da5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75023a9d0da5