Skip to content

[MSE][GStreamer] update maximum buffer size after receiving first init segment #1534

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: wpe-2.46
Choose a base branch
from
Open

[MSE][GStreamer] update maximum buffer size after receiving first init segment #1534

wants to merge 1 commit into from

Conversation

emutavchi
Copy link
Collaborator

@emutavchi emutavchi commented Jul 14, 2025 •
edited by ews-bot-wpe-rdk
Loading

This fixes the configuration of the maximum buffer size of audio source buffers via 'MSE_MAX_BUFFER_SIZE' env variable.

05c7c22

Build-Tests Layout-Tests
✅ 🛠 wpe-246-amd64-build ✅ 🧪 wpe-246-amd64-layout
✅ 🛠 wpe-246-arm32-build ✅ 🧪 wpe-246-arm32-layout

@modeveci modeveci requested review from eocanha and calvaris July 14, 2025 12:25
@eocanha
Copy link
Member

eocanha commented Jul 14, 2025

I'll have a look at this.

@eocanha
Copy link
Member

eocanha commented Jul 15, 2025 •
edited
Loading

This change might not be enough, and this call to setMaximumBufferSize() would also need to be removed.

The logic for maximumSourceBufferSize says that, if there's a previous value, that one should be used instead of the platform supplied value. The initial call is setting that value. While the initial call would ask the platform implementation about the size and return the right size according to the MSE_MAX_BUFFER_SIZE env var, the type of the SourceBuffer (audio, video, text or a combination of them) might not be known at that time before data is appended. After that moment, the platform code wouldn't be ever asked again.

It might make more sense to never call setMaximumSourceBufferSize() from any place, so the m_maximumBufferSize always stays nullopt and the platform code is always asked, even later, in the middle of SourceBuffer's lifecycle.

eocanha
eocanha requested changes Jul 15, 2025
View reviewed changes
@eocanha
Copy link
Member

eocanha commented Jul 16, 2025

@emutavchi, I think I can work on this myself, ensure that my suggestion actually works properly, and directly submit the patch upstream for review, if you don't mind.

@emutavchi
Copy link
Collaborator Author

Hi @eocanha, I'm not sure I follow your reasoning here. As far as I can see, WebCore::SourceBuffer::m_maximumBufferSize is set only for testing purposes (from WebCore::Internals::setMaximumSourceBufferSize()). And it is supposed to override any settings/platform configuration.

The WebCore::SourceBufferPrivate::setMaximumBufferSize updates WebCore::SourceBufferEvictionData::maximumBufferSize each time it is called with a different value. The point of this PR is to update WebCore::SourceBufferEvictionData::maximumBufferSize when the source buffer receives its first init segment. so it can apply limits correctly.

@emutavchi
[MSE][GStreamer] update maximum buffer size after receiving first ini…
05c7c22 
…t segment

Platform can specify different limits depending on the
type of the source buffer.
@emutavchi emutavchi force-pushed the wpe-2.46-fix-mse-audio-limits branch from 289128c to 05c7c22 Compare July 16, 2025 13:01
@eocanha
Copy link
Member

eocanha commented Jul 16, 2025

Ok, I'll run the code, check how the actual values behave, elaborate my reasoning if it makes sense, and submit it upstream for review when I'm sure I understand what happens.

eocanha added a commit to eocanha/WebKit that referenced this pull request Jul 16, 2025
@eocanha
[MSE][GStreamer] update maximum buffer size after receiving first ini…
76e4095 
…t segment

https://bugs.webkit.org/show_bug.cgi?id=296059

Reviewed by NOBODY (OOPS!).

The current SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment() code only calls SourceBufferPrivate::setMaximumBufferSize() on new init segments when the segment has (at least) a video track. Maximum buffer size is managed with high granularity in WPE by using the MSE_MAX_BUFFER_SIZE environment variable. For that reason, that call must happen independently of the kind of tracks present (at least on the WPE platform). This will ensure that the proper frame eviction code in SourceBufferPrivate::setMaximumBufferSize() is called in all cases.

See: WebPlatformForEmbedded/WPEWebKit#1534

This patch calls SourceBufferPrivate::setMaximumBufferSize() inconditionally on the WPE platform.

Original author: Eugene Mutavchi <[email protected]>

* Source/WebCore/Modules/mediasource/SourceBuffer.cpp:
(WebCore::SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment): Disable the if, so the call happens inconditionally on WPE.
@eocanha eocanha mentioned this pull request Jul 16, 2025
Open
@eocanha
Copy link
Member

eocanha commented Jul 16, 2025

Added some printfs here to exactly understand what happens (without your patch) and got this, for reference:

### uint64_t WebCore::SourceBuffer::maximumBufferSize() const
### virtual size_t WebCore::SourceBufferPrivateGStreamer::platformMaximumBufferSize() const: bufferSize += 314572800 (because of video)
### virtual size_t WebCore::SourceBufferPrivateGStreamer::platformMaximumBufferSize() const: bufferSize += 31457280 (because of audio)
### virtual size_t WebCore::SourceBufferPrivateGStreamer::platformMaximumBufferSize() const: bufferSize += 1048576 (because of text)
### virtual size_t WebCore::SourceBufferPrivateGStreamer::platformMaximumBufferSize() const: Final bufferSize = 347078656
^^^ This is because there are no tracks when SourceBuffer is constructed, so the code chooses the largest (most conservative) size, as if there was the 3 kind of tracks in the SourceBuffer.
### uint64_t WebCore::SourceBuffer::maximumBufferSize() const: maximumBufferSize(): Returning platformMaximumBufferSize() = 347078656
### WebCore::SourceBuffer::SourceBuffer(WTF::Ref<WebCore::SourceBufferPrivate>&&, WebCore::MediaSource&): maximumBufferSize(): 347078656
### uint64_t WebCore::SourceBuffer::maximumBufferSize() const
### virtual size_t WebCore::SourceBufferPrivateGStreamer::platformMaximumBufferSize() const: bufferSize += 314572800 (because of video)
### virtual size_t WebCore::SourceBufferPrivateGStreamer::platformMaximumBufferSize() const: Final bufferSize = 314572800
^^^ Now we have a video track (and only that) and the size is adjusted accordingly
### uint64_t WebCore::SourceBuffer::maximumBufferSize() const: maximumBufferSize(): Returning platformMaximumBufferSize() = 314572800
### WTF::Ref<WTF::NativePromise<void, WebCore::PlatformMediaError> > WebCore::SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment(WebCore::SourceBufferPrivateClient::InitializationSegment&&): First init segment, setting maximumBufferSize to maximumBufferSize() = 314572800

That's when I realized that the maximumBufferSize was not cached, as I had initially thought. I think I get it now, sorry:

I confused the calls to SourceBuffer::m_private->setMaximumBufferSize() with calls to SourceBuffer::setMaximumBufferSize(). The latter remembers and caches the supplied size, overridding forever the result that SourceBuffer::maximumBufferSize() returns, while the former is basically a call to SourceBufferPrivate::setMaximumBufferSize(), which just triggers immediate eviction when the size is too low.

Having that into account, now I really understand the patch and can say it's right. I've submitted it upstream as https://bugs.webkit.org/show_bug.cgi?id=296059 / WebKit/WebKit#48121

Sorry for all the mess. 🙏🙏🙏

@eocanha eocanha added the upstream Related to an upstream bug (or should be at some point) label Jul 16, 2025
@eocanha eocanha self-requested a review July 16, 2025 15:37
eocanha
eocanha approved these changes Jul 16, 2025
View reviewed changes
Copy link
Member

@eocanha eocanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved (but don't merge. The right commit will be backported from upstream when it lands)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eocanha eocanha eocanha approved these changes

@calvaris calvaris Awaiting requested review from calvaris

Assignees

@modeveci modeveci

Labels
upstream Related to an upstream bug (or should be at some point) wpe-2.46
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emutavchi @eocanha @modeveci