Summary: | [GStreamer] Streaming aac/mp3 audio doesn't always work | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | bugzilla | ||||||||||
Component: | Media | Assignee: | Enrique Ocaña <eocanha> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | calvaris, cgarcia, commit-queue, eocanha, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, philipj, pnormand, sergio, vjaquez, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Other | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
bugzilla
2020-01-06 07:39:13 PST
Which version of webkitgtk are you using? I am using Midori v8.0 which uses webkit2gtk-4.0, according to what I can find in the build files. (In reply to bugzilla from comment #2) > webkit2gtk-4.0, according to what I can > find in the build files. That's not the version I'm looking for. What's the package version? 2.26.2 ? 2.16.6, I think, according to these lines in the make file: pkg_check_modules(DEPS_GTK REQUIRED gtk+-3.0>=3.12.0 webkit2gtk-4.0>=2.16.6 gcr-ui-3>=2.32 libpeas-gtk-1.0 json-glib-1.0>=0.12 libarchive ) No, that's the minimum version required, not necessarily the detected version... Can you attach the midori build logs? Or simply the output of: pkg-config --modversion webkit2gtk-4.0 pkg-config reports 2.26.2 I can reproduce the bug, the problem is that our httpsrc element quickly assumes those live stream have a content-length, hence EOS is pushed too early and playback stops :( This works-around the bug, but might introduce other layout test failures, so it should be properly investigated: diff --git a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp index 3dae81aa68d..6be6193ffd1 100644 --- a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp +++ b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp @@ -1171,10 +1171,10 @@ void CachedResourceStreamingClient::dataReceived(PlatformMediaResource&, const c if (priv->haveSize && (newPosition > priv->size)) { GST_DEBUG_OBJECT(src, "Got position previous estimated content size (%" G_GINT64_FORMAT " > %" G_GINT64_FORMAT ")", newPosition, priv->size); newSize = newPosition; - } else if (!priv->haveSize) { - GST_DEBUG_OBJECT(src, "Got initial response without Content-Length, assuming response size as duration."); - newSize = length; - priv->haveSize = true; + // } else if (!priv->haveSize) { + // GST_DEBUG_OBJECT(src, "Got initial response without Content-Length, assuming response size as duration."); + // newSize = length; + // priv->haveSize = true; } if (newSize) { Created attachment 390651 [details]
Patch
Created attachment 390656 [details]
Patch
Comment on attachment 390656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390656&action=review > LayoutTests/http/tests/media/video-no-content-length.html:22 > + testExpected("video.duration == Infinity", true); shouldn't this be testExpected(video.duration, Infinity) ? That way the expectation would make more sense. Created attachment 390660 [details]
Patch
Comment on attachment 390660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390660&action=review Looks like the new test times out on mac-wk1 and mac-wk2 > LayoutTests/http/tests/media/resources/video-no-content-length.php:10 > + header("Content-Type: video/mp4"); > + $fileName = "test.mp4"; > + } else if ($extension == 'ogv') { > + header("Content-Type: video/ogg"); 4 spaces indentation please ;) > LayoutTests/http/tests/media/video-no-content-length.html:8 > +<script src=../../media-resources/video-test.js></script> > +<script src=../../media-resources/media-file.js></script> > +<script> i think script tags usually go in the head? > LayoutTests/http/tests/media/video-no-content-length.html:17 > + video.src="http://127.0.0.1:8000/media/resources/video-no-content-length.php?video="+movie; missing spaces surrounding the = (In reply to Philippe Normand from comment #13) > Comment on attachment 390660 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390660&action=review > > Looks like the new test times out on mac-wk1 and mac-wk2 > Your new php won't work on macOS or iOS unless it supports range requests. LayoutTests/http/tests/media/resources/serve-video.php already supports omitting the 'Content-Length' header, so you should be able to add a new option to LayoutTests/http/tests/media/resources/load-video.php to enable it for your test instead of adding range support to the new script. (In reply to Eric Carlson from comment #14) > Your new php won't work on macOS or iOS unless it supports range requests. > > LayoutTests/http/tests/media/resources/serve-video.php already supports > omitting the 'Content-Length' header, so you should be able to add a new > option to LayoutTests/http/tests/media/resources/load-video.php to enable it > for your test instead of adding range support to the new script. Thanks for the suggestion, Eric. I've modified the test to use load-video.php here locally (patch not submitted yet) and now it "works" on macOS. However, I'm getting a finite duration there instead of Infinite. It might be because the test.mp4 file used is a standalone file, while the ideal test case should be a stream like the one used in the Breeze radio stream (see original bug report). If I open a webpage with that stream in Safari, I get NaN before starting playback and then Infinite after playback start. I should find a way to replicate that behaviour in the test or just skip it completely on Mac. Created attachment 392604 [details]
Patch
Looks like I was testing the wrong thing in the previous layout test. I was focusing too much on the duration, but that was only known after the video (now audio) had detected end-of-stream (EOS) and stopped. I had to do a lot of empirical tests with lengths, chunk sizes, Icy headers and other stuff, but in the end I've finally managed to simulate a real live stream in this patch by stalling the server and not providing all the data at once. On the previous GStreamer code, this was detected as EOS and the audio stopped, showing a duration smaller than the real one (or Infinity on a real live stream). I couldn't check for an exact currentTime because the Mac and GStreamer ports give different exact values. I just check if it's approximately higher than 6 sec and that fits all ports. This time I've double checked that the new test fails without the patch (on GStreamer, in Mac it already passed), passes with the patch, and the modifications in serve-video.php don't create any regression in video-play-waiting.html and video-play-stall.html in Mac (the other tests using stallOffset/stallDuration). Comment on attachment 392604 [details]
Patch
LGTM. Thanks Enrique
Comment on attachment 392604 [details] Patch Clearing flags on attachment: 392604 Committed r257977: <https://trac.webkit.org/changeset/257977> All reviewed patches have been landed. Closing bug. |