Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSE] Skip initial buffering rate computation #10793

Merged
merged 1 commit into from Mar 7, 2023

Conversation

eocanha
Copy link
Contributor

@eocanha eocanha commented Feb 28, 2023

1581c8a

[MSE] Skip initial buffering rate computation
https://bugs.webkit.org/show_bug.cgi?id=253074

Reviewed by Jer Noble.

m_timeOfBufferingMonitor is initialized in SourceBuffer at creation
time. If the first call to monitorBufferingRate() takes too much to
happen for whatever reason (eg: because the webpage takes too much to
perform the first append), the computed interval is already going to be
too big and generate an artificially low m_averageBufferRate.

To mitigate this, I've added code to initialize m_timeOfBufferingMonitor
to zero, detect that special value in monitorBufferingRate() and skip
the first time that the rate computation is done (just assigning the
"now" time to m_timeOfBufferingMonitor, so when the next update is done,
the interval is most realistic).

However, since monitorBufferingRate() calls are only done from
canPlayThroughRange() and that call might happen only once in some
layout tests and production scenarios, m_averageBufferRate might not
reach accurate values on time and make the test fail. To mitigate that,
I've also added extra calls every time a buffer is appended and every
time the append completes.

See: WebPlatformForEmbedded/WPEWebKit#928 (comment)

* Source/WebCore/Modules/mediasource/SourceBuffer.cpp: Initialize m_timeOfBufferingMonitor to zero, meaning "never updated before".
(WebCore::SourceBuffer::appendBuffer): Update monitorBufferingRate.
(WebCore::SourceBuffer::sourceBufferPrivateAppendComplete): UpdateMonitorBufferingRate.
(WebCore::SourceBuffer::monitorBufferingRate): Skip first m_averageBufferRate update.

Canonical link: https://commits.webkit.org/261328@main

b16b73b

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@eocanha eocanha self-assigned this Feb 28, 2023
@eocanha eocanha added the Media Bugs related to the HTML 5 Media elements. label Feb 28, 2023
@eocanha eocanha added the merging-blocked Applied to prevent a change from being merged label Feb 28, 2023
@eocanha
Copy link
Contributor Author

eocanha commented Feb 28, 2023

The patch as it is is causing 3 regressions in glib platforms:

  • media/media-source/media-source-canplaythrough-event.html
  • media/media-source/media-source-video-renders.html
  • media/media-source/media-source-webm-append-buffer-after-abort.html

I'm going to have a quick look into them before considering merging the patch.

@philn
Copy link
Member

philn commented Feb 28, 2023

Seems to break tests on mac ports too.

@eocanha eocanha removed the merging-blocked Applied to prevent a change from being merged label Mar 1, 2023
@eocanha
Copy link
Contributor Author

eocanha commented Mar 1, 2023

The problem seemed to be that just one call to monitorBufferingRate() before the test checks canplaythrough isn't enough to get the new mechanism working (2 calls at minimum are needed). I've added extra calls every time an append is done and everytime an append is completed. That solved the issues on WebKitGTK. Let's see if it solves them too for Apple ports.

@eocanha
Copy link
Contributor Author

eocanha commented Mar 1, 2023

@jernoble, if you confirm that you don't see anything wrong about calling monitorBufferingRate() from SourceBuffer::appendBuffer() and SourceBuffer::sourceBufferPrivateAppendComplete() to force a more frequent update of m_averageBufferRate, I can merge the patch. All the tests are passing now.

@eocanha
Copy link
Contributor Author

eocanha commented Mar 3, 2023

@jernoble, in case there's no complain from your side in some days, I'll merge this PR next week.

@eocanha eocanha added the merge-queue Applied to send a pull request to merge-queue label Mar 7, 2023
https://bugs.webkit.org/show_bug.cgi?id=253074

Reviewed by Jer Noble.

m_timeOfBufferingMonitor is initialized in SourceBuffer at creation
time. If the first call to monitorBufferingRate() takes too much to
happen for whatever reason (eg: because the webpage takes too much to
perform the first append), the computed interval is already going to be
too big and generate an artificially low m_averageBufferRate.

To mitigate this, I've added code to initialize m_timeOfBufferingMonitor
to zero, detect that special value in monitorBufferingRate() and skip
the first time that the rate computation is done (just assigning the
"now" time to m_timeOfBufferingMonitor, so when the next update is done,
the interval is most realistic).

However, since monitorBufferingRate() calls are only done from
canPlayThroughRange() and that call might happen only once in some
layout tests and production scenarios, m_averageBufferRate might not
reach accurate values on time and make the test fail. To mitigate that,
I've also added extra calls every time a buffer is appended and every
time the append completes.

See: WebPlatformForEmbedded/WPEWebKit#928 (comment)

* Source/WebCore/Modules/mediasource/SourceBuffer.cpp: Initialize m_timeOfBufferingMonitor to zero, meaning "never updated before".
(WebCore::SourceBuffer::appendBuffer): Update monitorBufferingRate.
(WebCore::SourceBuffer::sourceBufferPrivateAppendComplete): UpdateMonitorBufferingRate.
(WebCore::SourceBuffer::monitorBufferingRate): Skip first m_averageBufferRate update.

Canonical link: https://commits.webkit.org/261328@main
@webkit-commit-queue
Copy link
Collaborator

Committed 261328@main (1581c8a): https://commits.webkit.org/261328@main

Reviewed commits have been landed. Closing PR #10793 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
5 participants