Bug 233495 - [MSE][WPE] Parameterize maximum buffer size using the MSE_MAX_BUFFER_SIZE env var
Summary: [MSE][WPE] Parameterize maximum buffer size using the MSE_MAX_BUFFER_SIZE env...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrique Ocaña
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-25 06:51 PST by Enrique Ocaña
Modified: 2021-12-01 02:57 PST (History)
17 users (show)

See Also:


Attachments
Patch (8.07 KB, patch)
2021-11-26 11:48 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (8.53 KB, patch)
2021-11-30 06:20 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (8.44 KB, patch)
2021-12-01 02:18 PST, Enrique Ocañ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 Enrique Ocaña 2021-11-25 06:51:32 PST
The current MSE policy of SourceBuffers taking all the available system memory until the MemoryPressureHandler complains may work fine for a typical desktop system, when caching is preferred over memory footprint. However, memory is scarce on embedded systems and WebKit may compete with other software for memory resources. In that scenario, the embedder may prefer to restrict the amount of memory available for each kind of SourceBuffer (audio, video, text).

Metrological has been successfully using an environment variable for that purpose downstream since 2016. If that implementation is upstreamed, other WPE users can benefit from it.
Comment 1 Enrique Ocaña 2021-11-26 11:48:56 PST
Created attachment 445206 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2021-11-30 02:46:51 PST
Comment on attachment 445206 [details]
Patch

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

I think we should move this logic to a virtual method, maybe in SourceBufferPrivate.

> Source/WebCore/html/MediaElementSession.cpp:946
> +    String s(getenv("MSE_MAX_BUFFER_SIZE"));

std::
Comment 3 Enrique Ocaña 2021-11-30 06:20:37 PST
Created attachment 445411 [details]
Patch
Comment 4 Enrique Ocaña 2021-11-30 06:32:14 PST
I discussed with Alicia the best place to have the logic and we realized that  MediaSessionManagerGLib might also be a good place for the platform logic. SourceBuffer asks HTMLMediaElement, which asks MediaElementSession, a subclass of PlatformMediaSession, which can talk to PlatformMediaSessionManager, which is subclassed by MediaSessionManagerGLib. 

However, all this Session stuff is related to attributes that can change and be set at runtime, while this platformMaximumBufferSize() is something more static, related to the platform. That's why I still think that SourceBufferPrivate should be its proper place to be.
Comment 5 Xabier Rodríguez Calvar 2021-12-01 01:08:44 PST
Comment on attachment 445411 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        * platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp: Read the MSE_MAX_BUFFER_SIZE environment variable and compute the maximum buffer size if the size is specified in the variable for all the track types present in the SourceBufferPrivate. Otherwise, use the default size.

This line is too long.

> Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:285
> +static void maximumBufferSizeDefaults(size_t& maxBufferSizeVideo, size_t& maxBufferSizeAudio, size_t& maxBufferSizeText)

If this function is going to operate on statics, I don't think you need the parameters, just use the statics.

Besides, I think considering this function is only called inside de std::once call, you could just inline the code there. You would even save a #ifdef.
Comment 6 Enrique Ocaña 2021-12-01 02:18:48 PST
Created attachment 445543 [details]
Patch
Comment 7 EWS 2021-12-01 02:56:29 PST
Committed r286358 (244717@main): <https://commits.webkit.org/244717@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445543 [details].
Comment 8 Radar WebKit Bug Importer 2021-12-01 02:57:20 PST
<rdar://problem/85914688>