Bug 194220 – [MSE][GStreamer] Batch player duration updates

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

This saves up a ton of CPU cycles doing layout unnecessarily when all
the appended frames extend the duration of the movie, like in
YTTV 2018 59.DASHLatencyVP9.

[Alicia Boya García]

Created attachment 361314 [details]
Patch

[Alicia Boya García]

Created attachment 361317 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 361317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361317&action=review

Apart from the specific comments, as Charlie, I think this a very convoluted and weird way to accomplish the goal.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:65
> +void
> +CodedFrameProcessingBatchContext::durationUpdate(RefPtr<PlaybackPipeline> playbackPipeline, MediaPlayer* mediaPlayer)

style and ASSERT for main thread.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:79
> +    ASSERT(!s_instance);

I know we're doing this in the caller as well but I think we should also ASSERT(isMainThread()) here to ensure we don't create two instances at the same time.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:93
> +void
> +CodedFrameProcessingBatchContext::enqueueDurationUpdate(RefPtr<PlaybackPipeline> playbackPipeline, MediaPlayer* mediaPlayer)

style

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:49
> +class CodedFrameProcessingBatchContext {

WTF_FORBID_HEAP_ALLOCATION;

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:51
> +    static void durationUpdate(RefPtr<PlaybackPipeline>, MediaPlayer*);

updateDuration

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:65
> +    MediaPlayer* m_mediaPlayer { nullptr };

You can probably make this a MediaPlayer& and take that as parameter in the enqueue and updateDuration as well.

[Alicia Boya García]

Comment on attachment 361317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361317&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:65
>> +    MediaPlayer* m_mediaPlayer { nullptr };
> 
> You can probably make this a MediaPlayer& and take that as parameter in the enqueue and updateDuration as well.

MediaPlayer* is provided by the player when calling durationUpdate(), not in the constructor. The only way to change that would be to initialize these variables with the creation of the context inside AppendPipeline. That would be doable, but I think it's clearer this way since you can see in the durationUpdate() call what objects will be acted upon.

[Alicia Boya García]

Regarding concerns of this being convoluted: the problem arises from the fact that duration updates are requested as part of sourceBufferPrivateDidReceiveSample(), which by design is called once per sample and will call durationChanged() if such sample grows the duration.

Therefore, when processing a batch of samples, we need a way to turn off the expensive notifications until the batch is finished. A stack context class like this does exactly that, in a RAII fashion.

[Xabier Rodríguez Calvar]

(In reply to Alicia Boya García from comment #4)
> Comment on attachment 361317 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361317&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:65
> >> +    MediaPlayer* m_mediaPlayer { nullptr };
> > 
> > You can probably make this a MediaPlayer& and take that as parameter in the enqueue and updateDuration as well.
> 
> MediaPlayer* is provided by the player when calling durationUpdate(), not in
> the constructor. The only way to change that would be to initialize these
> variables with the creation of the context inside AppendPipeline. That would
> be doable, but I think it's clearer this way since you can see in the
> durationUpdate() call what objects will be acted upon.

Yes, you'right!

[Alicia Boya García]

Comment on attachment 361317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361317&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:49
>> +class CodedFrameProcessingBatchContext {
> 
> WTF_FORBID_HEAP_ALLOCATION;

I would like to, but unfortunately it has the side effect of disallowing destructors.

[Alicia Boya García]

Created attachment 362380 [details]
Patch

[Alicia Boya García]

Created attachment 362381 [details]
Patch

[Alicia Boya García]

Created attachment 362607 [details]
Patch

[EWS Watchlist]

Comment on attachment 362607 [details]
Patch

Attachment 362607 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11232741

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 362616 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Alicia Boya García]

Created attachment 362652 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 362607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362607&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:680
> +    m_durationChangedWhileBlocked = false;

You can do this inside the if.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:118
> +    bool m_blockingDurationChanges = false;
> +    bool m_durationChangedWhileBlocked = false;

m_areDurationChangesBlocked and m_shouldReportDurationWhenUnblocking

[Alicia Boya García]

Created attachment 362742 [details]
Patch

[EWS Watchlist]

Comment on attachment 362742 [details]
Patch

Attachment 362742 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11257923

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 362818 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Alicia Boya García]

Created attachment 362824 [details]
Patch

[Charlie Turner]

Informal r+ from me :)

[WebKit Commit Bot]

Comment on attachment 362824 [details]
Patch

Clearing flags on attachment: 362824

Committed r242034: <https://trac.webkit.org/changeset/242034>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.