WebPlatformForEmbedded / WPEWebKit

WPE WebKit port (downstream)
211 stars 135 forks source link

[wpe-2.38] Fix playback position during simulated preroll #1250

Closed Scony closed 5 months ago

Scony commented 8 months ago

When simulating preroll in MSE, the player sets m_isSeeking = false immediately despite the actual preroll may not be done yet and hence playback position may not yield correct results at that time.

CC @calvaris

calvaris commented 8 months ago

On my to-do list.

calvaris commented 6 months ago

@Scony Please, provide an example in the form of an HTML so that I can have a proper look.

Scony commented 6 months ago

As I lost access to the internal Comcast tickets, @asurdej-comcast or @filipe-norte-red could you please take a look at the ticket linked to PR 1250 to check the precise test case failing?

calvaris commented 6 months ago

As I lost access to the internal Comcast tickets, @asurdej-comcast or @filipe-norte-red could you please take a look at the ticket linked to PR 1250 to check the precise test case failing?

Please, folks, I'm waiting for the use case to be able to check this.

asurdej-comcast commented 6 months ago

@calvaris Sorry for the delay, I'm on this right now but there is no clear test case available so I'm evaluating original case from Spotify as we had couple of connected tickets from this app

asurdej-comcast commented 6 months ago

Here you can find a test case: https://asurdej-comcast.github.io/tr/tests/RDK-43828_mse_audio_seek_into_unbuffered_while_seeking.html

In audio-only MSE seek we do gst_seek() + MSE seek. Seek is considered finished when ASYNC_DONE message is received from gst pipeline. But if we have non-async audio sink then we have this simulated preroll that triggers didPreroll() immediately after posting the seek (while MSE and gst are seeking still). Any position querry to the player will return invalid value until new data will reach audio sink.

asurdej-comcast commented 6 months ago

This is also visible in the logs:

0:00:05.732472760    35 0x55e0a2e9f210 DEBUG              webkitmse MediaPlayerPrivateGStreamerMSE.cpp:343:didPreroll: Pipeline prerolled. currentMediaTime = {18}
0:00:05.732507726    35 0x55e0a2e9f210 DEBUG              webkitmse MediaPlayerPrivateGStreamerMSE.cpp:356:didPreroll: Seek complete because of preroll. currentMediaTime = {5193998000/1000000000 = 5.193998}

Both are printed from the same cycle while first one is currentMediaTime() with m_isSeeking=true while the second one is after resetting seeking flag. Second position is also propagated to html media element with timeChanged() event

@calvaris

asurdej-comcast commented 5 months ago

@calvaris It looks like the same was changed recently by https://github.com/WebPlatformForEmbedded/WPEWebKit/pull/1211 Evaluating if the fix is good enough for our use-cases

calvaris commented 5 months ago

With default configuration both in the rpi3 and the reference box, I am getting a quota exceeded error during the last append. Is that the error? I don't think so.

asurdej-comcast commented 5 months ago

No, the quota error is not expected. I think this test case appends whole audio file that is like 3MB or so maybe you need to increase MSE buffering limits.

But with the change from https://github.com/WebPlatformForEmbedded/WPEWebKit/pull/1211 the test case works fine, tested with Spotify and it looks good also. So maybe we don't need this PR (1250) at all.

Once I have builds ready I will ask our QA to test more audio-only apps to confirm

calvaris commented 5 months ago

Please let me know if I can close this then.

calvaris commented 5 months ago

Well, from that test I see the position changing to 18 + some milliseconds, which is logical because it is playing and after the seek, the position can change. If you pause, position will be 18 as expected. I see nothing wrong here and hence I am closing the PR. Please reopen if there are new findings.