WebPlatformForEmbedded / WPEWebKit

WPE WebKit port (downstream)
214 stars 136 forks source link

After SourceBuffer::abort() add resetting parser to the last appended initialization segment. #1016

Closed jaroslaw-bojko-red closed 1 year ago

jaroslaw-bojko-red commented 1 year ago

In MSE playback, after receiving SourceBuffer::abort() we should execute the steps described in https://w3c.github.io/media-source/#dom-sourcebuffer-abort, including point 5. Run the reset parser state algorithm. This point specifies the exact variable that must be changed to reset the parser. This point does not mention the clearing init segment that the player previously received. The backend has to be ready for audio, video data, but still, it is obliged to remember decoder settings. (For example, different point: changeType directly specifies to abandon init segment because the playback settings changed)

We encounter this problem in VTM Go streaming where Shaka player executes: SourceBuffer::abort() and, after this, tries to continue the playback by sending later audio, video segments. Unfortunately, it fails as GStreamer expects a new init segment.

Simplified TC: first.html.txt show the problem. After abort() the player should still be able to play /1/0004.m4s and /1/0005.m4s, it indeed happens on x86 with Chromium or Firefox, but not on the WpeWebbrowser.

The second TC second.html.txt (with additional sending init segment after abort()) works on all browsers (but still, according to https://w3c.github.io/media-source specification, it should work without an extra init segment)

The same problem is described in https://bugs.webkit.org/show_bug.cgi?id=135164, along with the proposed patch to solve this. Unfortunately, the solution is for SourceBufferPrivateAVFObjC classes not used on the RDK board.

In Gstreamer, qtdemux.c after abort() changes its state from PAUSED to READE in gst_qtdemux_change_state, which invokes gst_qtdemux_reset (qtdemux, TRUE);. This method resets the parser with forgetting the init segment.

The TC was checked on our box (EosV1) and RPI3 with WPE2.22 image: https://drive.google.com/file/d/1Bwe1Uws3u46iiV69r9oPSpQcT0gXiElW/view?usp=sharing

rychem commented 1 year ago

Hi @eocanha Would you please share, whether you had chance to check the issue?

eocanha commented 1 year ago

Not yet. It's in my to-do list after https://github.com/WebPlatformForEmbedded/WPEWebKit/issues/928 and after any Pull Request that may appear (currently none on queue). If @modeveci considers it's appropriate, I can raise the priority level of this issue, though.

eocanha commented 1 year ago

I've been working at this for some days already, have found an apparent solution that works for your test case, was going to submit it as part of https://bugs.webkit.org/show_bug.cgi?id=228820 but realized that the layout tests mentioned there aren't passing. This needs further debugging.

eocanha commented 1 year ago

Found why the layout tests don't pass (qtdemux can't recover from an aborted partial data fragment) and still decided to submit the patch as https://bugs.webkit.org/show_bug.cgi?id=228820 / https://github.com/WebKit/WebKit/pull/11313 because it improves the current situation and would be enough to solve this Issue.

jaroslaw-bojko-red commented 1 year ago

@eocanha Thank you for the fast response and patch. I wanted to do a fast check this fix on vtm go (the application, where we discovered the problem), but the patch does not apply cleanly to wpe_2.22. Do you plan to backport this fix to wpe_2.22 and wpe_2.28?

eocanha commented 1 year ago

I've ported them provisionally to the following debug branches for you to test for any unexpected side effect:

If it causes trouble, you might need to consider to apply them locally before we find a more stable way to get the feature implemented.

jaroslaw-bojko-red commented 1 year ago

@eocanha I checked the fix on our wpe-2.22 and on vtm go (site where we noticed the issue) the problem is gone. The playback succesfully continues, also the TC from this issue passes.

I provided the build to our tester to check for other possible regressions I will let you know after the testing is complete.

eocanha commented 1 year ago

I'm not still completely comfident about the patch, because it breaks aborts on partial fragments: If you have a fragment of media but you don't append it completely (just a part of it, with the intention of appending the rest in a later append) and then abort(), the demuxer doesn't reset itself completely. It remains in a state where it expects the next data to be the continuation of the half-appended one, and if it's not, a parse error is triggered and the whole new append operation errors out. I fear that this can cause problems in other apps.

In case you still need this patch, would it be possible for you to apply it only on your production environment by now, instead of having it as an official patch in the wpe-2.22, 2.28 and 2.36 branches?

jaroslaw-bojko-red commented 1 year ago

@eocanha Our internal tests are still ongoing.

About the patch, it indeed fixes the problem with VTM go, but I think that without the full patch, the problem may return soon as according to https://w3c.github.io/media-source/#dom-sourcebuffer-abort the abort can be executed when buffer append algorithm is running. In such cache, I expect the fail as we would not do abort() between full segments. (Or in other cases that do not use the MSE) Do you plan to implement the patch with the support of aborts on partial fragments?

eocanha commented 1 year ago

Yes, I would like to implement proper support by improving how qtdemux resets its internal state on flushes, but it won't be an easy/quick task. Qtdemux is a hard to tame monster of 15K lines of code. 😕

eocanha commented 1 year ago

Just to keep you updated on this task, I've found the proper way to implement aborts (even after appending partial fragments) without the need for a new init segment. I'm now working on backporting the needed GStreamer patches and the corresponding WebKit patch to our buildroot and wpe-2.22, wpe-2.28 and wpe-2.38. I'm solving conflicts and trying not to break anything in the process.

eocanha commented 1 year ago

Closing Issue.

eocanha commented 1 year ago

BTW, don't forget to apply the GStreamer commits to your own versions of GStreamer or else the WPE commits will make things worse!

jaroslaw-bojko-red commented 1 year ago

@eocanha Thank you for the fix. We used it in the browser, testers confirmed that it does not introduce regressions.

rychem commented 1 year ago

Thank you @eocanha for your great support.