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@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);