Bug 194220 - [MSE][GStreamer] Batch player duration updates
Summary: [MSE][GStreamer] Batch player duration updates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-04 08:13 PST by Alicia Boya García
Modified: 2019-02-25 02:12 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.45 KB, patch)
2019-02-06 12:19 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (6.44 KB, patch)
2019-02-06 12:40 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2019-02-19 04:34 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2019-02-19 05:14 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (5.60 KB, patch)
2019-02-21 08:09 PST, Alicia Boya García
calvaris: review+
calvaris: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.47 MB, application/zip)
2019-02-21 10:08 PST, Build Bot
no flags Details
Patch (5.54 KB, patch)
2019-02-21 15:12 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (5.59 KB, patch)
2019-02-22 12:18 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.74 MB, application/zip)
2019-02-23 00:53 PST, Build Bot
no flags Details
Patch (5.59 KB, patch)
2019-02-23 04:32 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2019-02-04 08:13:21 PST
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.
Comment 1 Alicia Boya García 2019-02-06 12:19:14 PST
Created attachment 361314 [details]
Patch
Comment 2 Alicia Boya García 2019-02-06 12:40:46 PST
Created attachment 361317 [details]
Patch
Comment 3 Xabier Rodríguez Calvar 2019-02-07 03:25:34 PST
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.
Comment 4 Alicia Boya García 2019-02-18 04:55:31 PST
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.
Comment 5 Alicia Boya García 2019-02-18 05:01:01 PST
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.
Comment 6 Xabier Rodríguez Calvar 2019-02-18 22:57:35 PST
(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!
Comment 7 Alicia Boya García 2019-02-19 04:09:50 PST
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.
Comment 8 Alicia Boya García 2019-02-19 04:34:29 PST
Created attachment 362380 [details]
Patch
Comment 9 Alicia Boya García 2019-02-19 05:14:50 PST
Created attachment 362381 [details]
Patch
Comment 10 Alicia Boya García 2019-02-21 08:09:33 PST
Created attachment 362607 [details]
Patch
Comment 11 Build Bot 2019-02-21 10:08:15 PST
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
Comment 12 Build Bot 2019-02-21 10:08:16 PST
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
Comment 13 Alicia Boya García 2019-02-21 15:12:18 PST
Created attachment 362652 [details]
Patch
Comment 14 Xabier Rodríguez Calvar 2019-02-22 03:41:39 PST
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
Comment 15 Alicia Boya García 2019-02-22 12:18:39 PST
Created attachment 362742 [details]
Patch
Comment 16 Build Bot 2019-02-23 00:53:14 PST
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
Comment 17 Build Bot 2019-02-23 00:53:16 PST
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
Comment 18 Alicia Boya García 2019-02-23 04:32:14 PST
Created attachment 362824 [details]
Patch
Comment 19 Charlie Turner 2019-02-24 10:01:16 PST
Informal r+ from me :)
Comment 20 WebKit Commit Bot 2019-02-25 02:12:03 PST
Comment on attachment 362824 [details]
Patch

Clearing flags on attachment: 362824

Committed r242034: <https://trac.webkit.org/changeset/242034>
Comment 21 WebKit Commit Bot 2019-02-25 02:12:05 PST
All reviewed patches have been landed.  Closing bug.