Bug 202204 - REGRESSION(r246399): [GStreamer] Problems playing AAC streams
Summary: REGRESSION(r246399): [GStreamer] Problems playing AAC streams
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks: 198377
  Show dependency treegraph
 
Reported: 2019-09-25 07:44 PDT by Alberto Garcia
Modified: 2019-10-09 01:18 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.76 KB, patch)
2019-10-07 09:11 PDT, Philippe Normand
calvaris: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alberto Garcia 2019-09-25 07:44:54 PDT
This stream fails to play, or plays only for a few seconds:

http://tmp.xaq.nl/test-aac.html

These two work fine, but will occasionally fail if played *after* the AAC stream:

http://tmp.xaq.nl/test-mp3.html
http://tmp.xaq.nl/test-hls.html

Reportedly WebKitGTK 2.24.0 was the last one that didn't have problems with this.

I can reproduce the problem with 2.24.4, and it seems to fail with 2.26.1 as well.

Originally reported here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=941137
Comment 1 Carlos Alberto Lopez Perez 2019-09-30 13:08:16 PDT
I can't reproduce with 2.24.2, but I can with 2.24.3.
I can also reproduce with trunk.

So I guess the problem is caused by one of the patches back-ported to 2.24.3.
Looking at the list https://trac.webkit.org/wiki/WebKitGTK/2.24.x#Proposedmergesfor2.24.3 I see several GStreamer related patches there.
Comment 2 Carlos Alberto Lopez Perez 2019-09-30 13:15:21 PDT
It seems it is caused by r246399

If I revert it on my local checkout (I'm on trunk@250519) I can't longer reproduce the issue.
Comment 3 Philippe Normand 2019-10-01 00:53:50 PDT
I'll try to check it this week, thanks for the bisect Carlos!
Comment 4 Philippe Normand 2019-10-01 05:06:43 PDT
Strange that commit introduced the regression for sure?

Here what I observe is that the internal queue gets drained just before the next data chunk arrives and that triggers EOS. However it shouldn't be triggered because this is a "live" non-seekable stream. I have a simple fix but I'll try to write a layout test as well, which will delay the upstreaming of the fix.

So here's the fix, in case downstream users are in urgent need of it:

diff --git a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
index 9810c257b37..965b6e276d0 100644
--- a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
+++ b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
@@ -451,11 +451,11 @@ static GstFlowReturn webKitWebSrcCreate(GstPushSrc* pushSrc, GstBuffer** buffer)
             GST_BUFFER_OFFSET(*buffer) = baseSrc->segment.position;
             GST_BUFFER_OFFSET_END(*buffer) = GST_BUFFER_OFFSET(*buffer) + size;
             GST_TRACE_OBJECT(src, "Buffer bounds set to %" G_GUINT64_FORMAT "-%" G_GUINT64_FORMAT, GST_BUFFER_OFFSET(*buffer), GST_BUFFER_OFFSET_END(*buffer));
-            GST_TRACE_OBJECT(src, "doesHaveEOS: %s, wasSeeking: %s, seeking: %s, size: %u", boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->wasSeeking), boolForPrinting(priv->isSeeking), size);
+            GST_TRACE_OBJECT(src, "doesHaveEOS: %s, wasSeeking: %s, seeking: %s, buffer size: %u, size: %" G_GUINT64_FORMAT, boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->wasSeeking), boolForPrinting(priv->isSeeking), size, priv->size);
             if (priv->haveSize && GST_BUFFER_OFFSET_END(*buffer) >= priv->size) {
                 if (priv->wasSeeking)
                     priv->wasSeeking = false;
-                else
+                else if (priv->isSeekable)
                     priv->doesHaveEOS = true;
             } else if (priv->wasSeeking)
                 priv->wasSeeking = false;
Comment 5 Carlos Alberto Lopez Perez 2019-10-01 05:30:33 PDT
(In reply to Philippe Normand from comment #4)
> Strange that commit introduced the regression for sure?
> 

I'm not sure if it was the one that introduced the regression, or the one that seems to work-around the issue if reverted. I didn't a full bisect.
I just tested to revert it on my local branch, and observed that after reverting it the issue seems fixed.

> Here what I observe is that the internal queue gets drained just before the
> next data chunk arrives and that triggers EOS. However it shouldn't be
> triggered because this is a "live" non-seekable stream. I have a simple fix
> but I'll try to write a layout test as well, which will delay the
> upstreaming of the fix.
> 
> So here's the fix, in case downstream users are in urgent need of it:
> 
> diff --git
> a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
> b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
> index 9810c257b37..965b6e276d0 100644
> --- a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
> +++ b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
> @@ -451,11 +451,11 @@ static GstFlowReturn webKitWebSrcCreate(GstPushSrc*
> pushSrc, GstBuffer** buffer)
>              GST_BUFFER_OFFSET(*buffer) = baseSrc->segment.position;
>              GST_BUFFER_OFFSET_END(*buffer) = GST_BUFFER_OFFSET(*buffer) +
> size;
>              GST_TRACE_OBJECT(src, "Buffer bounds set to %" G_GUINT64_FORMAT
> "-%" G_GUINT64_FORMAT, GST_BUFFER_OFFSET(*buffer),
> GST_BUFFER_OFFSET_END(*buffer));
> -            GST_TRACE_OBJECT(src, "doesHaveEOS: %s, wasSeeking: %s,
> seeking: %s, size: %u", boolForPrinting(priv->doesHaveEOS),
> boolForPrinting(priv->wasSeeking), boolForPrinting(priv->isSeeking), size);
> +            GST_TRACE_OBJECT(src, "doesHaveEOS: %s, wasSeeking: %s,
> seeking: %s, buffer size: %u, size: %" G_GUINT64_FORMAT,
> boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->wasSeeking),
> boolForPrinting(priv->isSeeking), size, priv->size);
>              if (priv->haveSize && GST_BUFFER_OFFSET_END(*buffer) >=
> priv->size) {
>                  if (priv->wasSeeking)
>                      priv->wasSeeking = false;
> -                else
> +                else if (priv->isSeekable)
>                      priv->doesHaveEOS = true;
>              } else if (priv->wasSeeking)
>                  priv->wasSeeking = false;

I tested this patch and it also fixes the issue :)
Comment 6 Philippe Normand 2019-10-07 09:11:03 PDT
Created attachment 380332 [details]
Patch
Comment 7 Philippe Normand 2019-10-07 09:12:33 PDT
I haven't managed to write a reliable test unfortunately.
Comment 8 Philippe Normand 2019-10-09 01:18:00 PDT
Committed r250901: <https://trac.webkit.org/changeset/250901>