Summary: | [MSE][GStreamer] Batch player duration updates | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alicia Boya García <aboya> | ||||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Alicia Boya García <aboya> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, commit-queue, cturner, ews-watchlist | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Alicia Boya García
2019-02-04 08:13:21 PST
Created attachment 361314 [details]
Patch
Created attachment 361317 [details]
Patch
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 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. 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. (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 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. Created attachment 362380 [details]
Patch
Created attachment 362381 [details]
Patch
Created attachment 362607 [details]
Patch
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 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
Created attachment 362652 [details]
Patch
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 Created attachment 362742 [details]
Patch
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 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
Created attachment 362824 [details]
Patch
Informal r+ from me :) Comment on attachment 362824 [details] Patch Clearing flags on attachment: 362824 Committed r242034: <https://trac.webkit.org/changeset/242034> All reviewed patches have been landed. Closing bug. |