[PATCH v2] winegstreamer: Split audio data to fit in the sample buffer.

[Anton Baskanov]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50355
Signed-off-by: Anton Baskanov <baskanov(a)gmail.com>
---
v2: Set discontinuity and sync point flags only for the first sample.
---
 dlls/winegstreamer/gstdemux.c | 144 +++++++++++++++++++++-------------
 1 file changed, 90 insertions(+), 54 deletions(-)

diff --git a/dlls/winegstreamer/gstdemux.c b/dlls/winegstreamer/gstdemux.c
index 9114e6610a3..676b7396515 100644
--- a/dlls/winegstreamer/gstdemux.c
+++ b/dlls/winegstreamer/gstdemux.c
@@ -81,6 +81,7 @@ struct gstdemux_source
     HANDLE caps_event, eos_event;
     GstSegment *segment;
     SourceSeeking seek;
+    DWORD bytes_per_second;
 };
 
 static inline struct gstdemux *impl_from_strmbase_filter(struct strmbase_filter *iface)
@@ -801,6 +802,7 @@ static GstFlowReturn got_data_sink(GstPad *pad, GstObject *parent, GstBuffer *bu
     BYTE *ptr = NULL;
     IMediaSample *sample;
     GstMapInfo info;
+    gsize position = 0;
 
     TRACE("%p %p\n", pad, buf);
 
@@ -809,76 +811,108 @@ static GstFlowReturn got_data_sink(GstPad *pad, GstObject *parent, GstBuffer *bu
         return GST_FLOW_OK;
     }
 
-    hr = BaseOutputPinImpl_GetDeliveryBuffer(&pin->pin, &sample, NULL, NULL, 0);
-
-    if (hr == VFW_E_NOT_CONNECTED) {
-        gst_buffer_unref(buf);
-        return GST_FLOW_NOT_LINKED;
-    }
+    gst_buffer_map(buf, &info, GST_MAP_READ);
 
-    if (FAILED(hr)) {
-        gst_buffer_unref(buf);
-        ERR("Could not get a delivery buffer (%x), returning GST_FLOW_FLUSHING\n", hr);
-        return GST_FLOW_FLUSHING;
-    }
+    while (position < info.size)
+    {
+        gsize advance;
 
-    gst_buffer_map(buf, &info, GST_MAP_READ);
+        hr = BaseOutputPinImpl_GetDeliveryBuffer(&pin->pin, &sample, NULL, NULL, 0);
 
-    hr = IMediaSample_SetActualDataLength(sample, info.size);
-    if(FAILED(hr)){
-        WARN("SetActualDataLength failed: %08x\n", hr);
-        return GST_FLOW_FLUSHING;
-    }
+        if (hr == VFW_E_NOT_CONNECTED) {
+            gst_buffer_unmap(buf, &info);
+            gst_buffer_unref(buf);
+            return GST_FLOW_NOT_LINKED;
+        }
 
-    IMediaSample_GetPointer(sample, &ptr);
+        if (FAILED(hr)) {
+            gst_buffer_unmap(buf, &info);
+            gst_buffer_unref(buf);
+            ERR("Could not get a delivery buffer (%x), returning GST_FLOW_FLUSHING\n", hr);
+            return GST_FLOW_FLUSHING;
+        }
 
-    memcpy(ptr, info.data, info.size);
+        advance = min(IMediaSample_GetSize(sample), info.size - position);
 
-    gst_buffer_unmap(buf, &info);
+        hr = IMediaSample_SetActualDataLength(sample, advance);
+        if(FAILED(hr)){
+            gst_buffer_unmap(buf, &info);
+            gst_buffer_unref(buf);
+            WARN("SetActualDataLength failed: %08x\n", hr);
+            return GST_FLOW_FLUSHING;
+        }
 
-    if (GST_BUFFER_PTS_IS_VALID(buf)) {
-        REFERENCE_TIME rtStart = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, buf->pts);
-        if (rtStart >= 0)
-            rtStart /= 100;
-
-        if (GST_BUFFER_DURATION_IS_VALID(buf)) {
-            REFERENCE_TIME tStart = buf->pts / 100;
-            REFERENCE_TIME tStop = (buf->pts + buf->duration) / 100;
-            REFERENCE_TIME rtStop;
-            rtStop = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, buf->pts + buf->duration);
-            if (rtStop >= 0)
-                rtStop /= 100;
-            TRACE("Current time on %p: %i to %i ms\n", pin, (int)(rtStart / 10000), (int)(rtStop / 10000));
-            IMediaSample_SetTime(sample, &rtStart, rtStop >= 0 ? &rtStop : NULL);
-            IMediaSample_SetMediaTime(sample, &tStart, &tStop);
+        IMediaSample_GetPointer(sample, &ptr);
+
+        memcpy(ptr, &info.data[position], advance);
+
+        if (GST_BUFFER_PTS_IS_VALID(buf)) {
+            REFERENCE_TIME rtStart;
+            GstClockTime ptsStart = buf->pts;
+            if (position > 0 && pin->bytes_per_second)
+                ptsStart = buf->pts + gst_util_uint64_scale(position, GST_SECOND, pin->bytes_per_second);
+            rtStart = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, ptsStart);
+            if (rtStart >= 0)
+                rtStart /= 100;
+
+            if (GST_BUFFER_DURATION_IS_VALID(buf)) {
+                REFERENCE_TIME rtStop;
+                REFERENCE_TIME tStart;
+                REFERENCE_TIME tStop;
+                GstClockTime ptsStop = buf->pts + buf->duration;
+                if (position + advance < info.size && pin->bytes_per_second)
+                    ptsStop = buf->pts + gst_util_uint64_scale(position + advance, GST_SECOND, pin->bytes_per_second);
+                tStart = ptsStart / 100;
+                tStop = ptsStop / 100;
+                rtStop = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, ptsStop);
+                if (rtStop >= 0)
+                    rtStop /= 100;
+                TRACE("Current time on %p: %i to %i ms\n", pin, (int)(rtStart / 10000), (int)(rtStop / 10000));
+                IMediaSample_SetTime(sample, &rtStart, rtStop >= 0 ? &rtStop : NULL);
+                IMediaSample_SetMediaTime(sample, &tStart, &tStop);
+            } else {
+                IMediaSample_SetTime(sample, rtStart >= 0 ? &rtStart : NULL, NULL);
+                IMediaSample_SetMediaTime(sample, NULL, NULL);
+            }
         } else {
-            IMediaSample_SetTime(sample, rtStart >= 0 ? &rtStart : NULL, NULL);
+            IMediaSample_SetTime(sample, NULL, NULL);
             IMediaSample_SetMediaTime(sample, NULL, NULL);
         }
-    } else {
-        IMediaSample_SetTime(sample, NULL, NULL);
-        IMediaSample_SetMediaTime(sample, NULL, NULL);
-    }
 
-    IMediaSample_SetDiscontinuity(sample, GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DISCONT));
-    IMediaSample_SetPreroll(sample, GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_LIVE));
-    IMediaSample_SetSyncPoint(sample, !GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DELTA_UNIT));
+        IMediaSample_SetDiscontinuity(sample,
+                !position && GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DISCONT));
+        IMediaSample_SetPreroll(sample, GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_LIVE));
+        IMediaSample_SetSyncPoint(sample,
+                !position && !GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DELTA_UNIT));
 
-    if (!pin->pin.pin.peer)
-        hr = VFW_E_NOT_CONNECTED;
-    else
-        hr = IMemInputPin_Receive(pin->pin.pMemInputPin, sample);
+        if (!pin->pin.pin.peer)
+            hr = VFW_E_NOT_CONNECTED;
+        else
+            hr = IMemInputPin_Receive(pin->pin.pMemInputPin, sample);
 
-    TRACE("sending sample returned: %08x\n", hr);
+        TRACE("sending sample returned: %08x\n", hr);
 
-    gst_buffer_unref(buf);
-    IMediaSample_Release(sample);
+        IMediaSample_Release(sample);
 
-    if (hr == VFW_E_NOT_CONNECTED)
-        return GST_FLOW_NOT_LINKED;
+        if (hr == VFW_E_NOT_CONNECTED)
+        {
+            gst_buffer_unmap(buf, &info);
+            gst_buffer_unref(buf);
+            return GST_FLOW_NOT_LINKED;
+        }
 
-    if (FAILED(hr))
-        return GST_FLOW_FLUSHING;
+        if (FAILED(hr))
+        {
+            gst_buffer_unmap(buf, &info);
+            gst_buffer_unref(buf);
+            return GST_FLOW_FLUSHING;
+        }
+
+        position += advance;
+    }
+
+    gst_buffer_unmap(buf, &info);
+    gst_buffer_unref(buf);
 
     return GST_FLOW_OK;
 }
@@ -2104,6 +2138,8 @@ static HRESULT WINAPI GSTOutPin_DecideBufferSize(struct strmbase_source *iface,
     {
         WAVEFORMATEX *format = (WAVEFORMATEX *)pin->pin.pin.mt.pbFormat;
         buffer_size = format->nAvgBytesPerSec;
+
+        pin->bytes_per_second = format->nAvgBytesPerSec;
     }
 
     props->cBuffers = max(props->cBuffers, 1);
-- 
2.25.1

[Zebediah Figura (she/her)]

If necessary I'm willing to sign off on the patch now, because it's code
freeze, but since there's probably still time I've inlined some comments.

I've also replaced the contents of the below message with the output of
"git diff --ignore-all-space" for clarity.

On 12/24/20 9:12 PM, Anton Baskanov wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50355
>     Signed-off-by: Anton Baskanov <baskanov(a)gmail.com>
>
> diff --git a/dlls/winegstreamer/gstdemux.c b/dlls/winegstreamer/gstdemux.c
> index 9114e6610a3..676b7396515 100644
> --- a/dlls/winegstreamer/gstdemux.c
> +++ b/dlls/winegstreamer/gstdemux.c
> @@ -81,6 +81,7 @@ struct gstdemux_source
>      HANDLE caps_event, eos_event;
>      GstSegment *segment;
>      SourceSeeking seek;
> +    DWORD bytes_per_second;
>  };
>
> static inline struct gstdemux *impl_from_strmbase_filter(struct strmbase_filter *iface)
> @@ -801,6 +802,7 @@ static GstFlowReturn got_data_sink(GstPad *pad, GstObject *parent, GstBuffer *bu
>      BYTE *ptr = NULL;
>      IMediaSample *sample;
>      GstMapInfo info;
> +    gsize position = 0;
>
> TRACE("%p %p\n", pad, buf);
>
> @@ -809,43 +811,60 @@ static GstFlowReturn got_data_sink(GstPad *pad, GstObject *parent, GstBuffer *bu
>          return GST_FLOW_OK;
>      }
>
> +    gst_buffer_map(buf, &info, GST_MAP_READ);
> +
> +    while (position < info.size)
> +    {
> +        gsize advance;
> +
>          hr = BaseOutputPinImpl_GetDeliveryBuffer(&pin->pin, &sample, NULL, NULL, 0);
>
> if (hr == VFW_E_NOT_CONNECTED) {
> +            gst_buffer_unmap(buf, &info);
>              gst_buffer_unref(buf);
>              return GST_FLOW_NOT_LINKED;
>          }
>
> if (FAILED(hr)) {
> +            gst_buffer_unmap(buf, &info);
>              gst_buffer_unref(buf);
>              ERR("Could not get a delivery buffer (%x), returning GST_FLOW_FLUSHING\n", hr);
>              return GST_FLOW_FLUSHING;
>          }
>
> -    gst_buffer_map(buf, &info, GST_MAP_READ);
> +        advance = min(IMediaSample_GetSize(sample), info.size - position);
>
> -    hr = IMediaSample_SetActualDataLength(sample, info.size);
> +        hr = IMediaSample_SetActualDataLength(sample, advance);
>          if(FAILED(hr)){
> +            gst_buffer_unmap(buf, &info);
> +            gst_buffer_unref(buf);

Nitpick, but this is a preëxisting bug and could be a separate patch.
> WARN("SetActualDataLength failed: %08x\n", hr);
>              return GST_FLOW_FLUSHING;
>          }
>
> IMediaSample_GetPointer(sample, &ptr);
>
> -    memcpy(ptr, info.data, info.size);
> -
> -    gst_buffer_unmap(buf, &info);
> +        memcpy(ptr, &info.data[position], advance);
>
> if (GST_BUFFER_PTS_IS_VALID(buf)) {
> -        REFERENCE_TIME rtStart = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, buf->pts);
> +            REFERENCE_TIME rtStart;
> +            GstClockTime ptsStart = buf->pts;
> +            if (position > 0 && pin->bytes_per_second)

I don't exactly like this. The "position > 0" part should be redundant
(and isn't worth including just to avoid an extra calculation), but what
bothers me more is the overloaded meaning of "pin->bytes_per_second".

In practice I don't think we should really be indiscriminately splitting
samples like this. We can't split e.g. video frames, for one. It's a bit
of a moot point, since a buffer too big to fit in one sample is a bug
*anyway*, but if we need to be able to scale by bit rate, we should
limit the splitting to circumstances where bit rate is actually valid,
which I think means only raw audio. (I'm not sure if it's also possible
for GStreamer to arbitrarily cut mp3 frames, but if it is, and we need
to worry about overlarge buffers, we'll need to find another way to
calculate the bit rate.)

Splitting the code like that would probably encourage the separation of
a helper function to do some of the work of filling and sending an
individual DirectShow sample. That of course would be a separate patch
if introduced.

Note that in that case the "bytes_per_second" field should probably go
away, and be replaced with direct access of nAvgBytesPerSec from
got_data_sink().
> +                ptsStart = buf->pts + gst_util_uint64_scale(position, GST_SECOND, pin->bytes_per_second);
> +            rtStart = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, ptsStart);
>              if (rtStart >= 0)
>                  rtStart /= 100;
>
> if (GST_BUFFER_DURATION_IS_VALID(buf)) {
> -            REFERENCE_TIME tStart = buf->pts / 100;
> -            REFERENCE_TIME tStop = (buf->pts + buf->duration) / 100;
>                  REFERENCE_TIME rtStop;
> -            rtStop = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, buf->pts + buf->duration);
> +                REFERENCE_TIME tStart;
> +                REFERENCE_TIME tStop;
> +                GstClockTime ptsStop = buf->pts + buf->duration;
> +                if (position + advance < info.size && pin->bytes_per_second)
> +                    ptsStop = buf->pts + gst_util_uint64_scale(position + advance, GST_SECOND, pin->bytes_per_second);
> +                tStart = ptsStart / 100;
> +                tStop = ptsStop / 100;
> +                rtStop = gst_segment_to_running_time(pin->segment, GST_FORMAT_TIME, ptsStop);
>                  if (rtStop >= 0)
>                      rtStop /= 100;
>                  TRACE("Current time on %p: %i to %i ms\n", pin, (int)(rtStart / 10000), (int)(rtStop / 10000));
> @@ -860,9 +879,11 @@ static GstFlowReturn got_data_sink(GstPad *pad, GstObject *parent, GstBuffer *bu
>              IMediaSample_SetMediaTime(sample, NULL, NULL);
>          }
>
> -    IMediaSample_SetDiscontinuity(sample, GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DISCONT));
> +        IMediaSample_SetDiscontinuity(sample,
> +                !position && GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DISCONT));

This looks correct, but...
> IMediaSample_SetPreroll(sample, GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_LIVE));
> -    IMediaSample_SetSyncPoint(sample, !GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DELTA_UNIT));
> +        IMediaSample_SetSyncPoint(sample,
> +                !position && !GST_BUFFER_FLAG_IS_SET(buf, GST_BUFFER_FLAG_DELTA_UNIT));

this looks wrong, or at least weird. It doesn't make sense in the first
place to mark a raw audio sample as a delta unit, but if hypothetically
there were a splittable sample which was a delta unit, I think it'd be
true that all of the pieces are delta units—none of them can be decoded
alone.
> if (!pin->pin.pin.peer)
>              hr = VFW_E_NOT_CONNECTED;
> @@ -871,14 +892,27 @@ static GstFlowReturn got_data_sink(GstPad *pad, GstObject *parent, GstBuffer *bu
>
> TRACE("sending sample returned: %08x\n", hr);
>
> -    gst_buffer_unref(buf);
>          IMediaSample_Release(sample);
>
> if (hr == VFW_E_NOT_CONNECTED)
> +        {
> +            gst_buffer_unmap(buf, &info);
> +            gst_buffer_unref(buf);
>              return GST_FLOW_NOT_LINKED;
> +        }
>
> if (FAILED(hr))
> +        {
> +            gst_buffer_unmap(buf, &info);
> +            gst_buffer_unref(buf);
>              return GST_FLOW_FLUSHING;
> +        }
> +
> +        position += advance;
> +    }
> +
> +    gst_buffer_unmap(buf, &info);
> +    gst_buffer_unref(buf);
>
> return GST_FLOW_OK;
>  }
> @@ -2104,6 +2138,8 @@ static HRESULT WINAPI GSTOutPin_DecideBufferSize(struct strmbase_source *iface,
>      {
>          WAVEFORMATEX *format = (WAVEFORMATEX *)pin->pin.pin.mt.pbFormat;
>          buffer_size = format->nAvgBytesPerSec;
> +
> +        pin->bytes_per_second = format->nAvgBytesPerSec;
>      }
>
> props->cBuffers = max(props->cBuffers, 1);