Changeset 284711 in webkit

Timestamp:

Oct 22, 2021, 1:22:52 PM (5 years ago)

Author:

aboya@igalia.com

Message:

[MSE][GStreamer] Honor MP4 edit lists, bis
​https://bugs.webkit.org/show_bug.cgi?id=231019

Reviewed by Xabier Rodriguez-Calvar.

Source/WebCore:

This patch takes into consideration the GstSegment attached to a
sample to offset the PTS and DTS. This ensures accurate timestamps are
obtained for MP4 files containing edit lists (commonly necessary for
files containing video with B frames to have PTS starting at zero).

Before this was implemented, a workaround was in place based on a
heuristic (DTS = 0 && PTS > 0 && PTS < 0.1). The workaround is
preserved for the sake of content without proper edit lists, but
any edit list takes preference.

The time fudge factor has been modified from 0.083 seconds up to
0.100 seconds to accomodate the size of the empty edit in test.mp4
used by Web Platform Tests.

This test fixes improves expectation results and fixes two subtests in
imported/w3c/web-platform-tests/media-source/mediasource-remove.html.

This is a reworked version that avoids using gst_sample_set_buffer()
which is not available on GStreamer 1.14, and fixes an issue where
frames that would get a negative DTS were not being enqueued properly.

• Modules/mediasource/MediaSource.cpp:

(WebCore::MediaSource::currentTimeFudgeFactor):

• platform/graphics/SourceBufferPrivate.h:

(WebCore::SourceBufferPrivate::timeFudgeFactor const):

• platform/graphics/gstreamer/GStreamerCommon.h:

(WebCore::toGstClockTime):

• platform/graphics/gstreamer/MediaSampleGStreamer.cpp:

(WebCore::MediaSampleGStreamer::MediaSampleGStreamer):

• platform/graphics/gstreamer/mse/AppendPipeline.cpp:

(WebCore::bufferTimeToStreamTime):
(WebCore::AppendPipeline::appsinkNewSample):

LayoutTests:

Update expectations for mediasource-remove.html in the GStreamer
ports, as a couple subtests get fixed.

• platform/glib/imported/w3c/web-platform-tests/media-source/mediasource-remove-expected.txt:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/glib/imported/w3c/web-platform-tests/media-source/mediasource-remove-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/Modules/mediasource/MediaSource.cpp (modified) (1 diff)
• Source/WebCore/platform/graphics/SourceBufferPrivate.h (modified) (1 diff)
• Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h (modified) (1 diff)
• Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp (modified) (1 diff)
• Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-10-22  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Honor MP4 edit lists, bis
+        https://bugs.webkit.org/show_bug.cgi?id=231019
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        Update expectations for mediasource-remove.html in the GStreamer
+        ports, as a couple subtests get fixed.
+
+        * platform/glib/imported/w3c/web-platform-tests/media-source/mediasource-remove-expected.txt:
+
 2021-10-22  Chris Dumez  <cdumez@apple.com>
 

trunk/LayoutTests/platform/glib/imported/w3c/web-platform-tests/media-source/mediasource-remove-expected.txt

@@ -12 +12 @@
 PASS Test remove with a start at the duration.
 PASS Test remove transitioning readyState from 'ended' to 'open'.
-FAIL Test removing all appended data. assert_equals: Initial buffered range. expected "{ [0.095, 6.548) }" but got "{ [0.000, 6.548) }"
-FAIL Test removing beginning of appended data. assert_equals: Initial buffered range. expected "{ [0.095, 6.548) }" but got "{ [0.000, 6.548) }"
-FAIL Test removing the middle of appended data. assert_equals: Initial buffered range. expected "{ [0.095, 6.548) }" but got "{ [0.000, 6.548) }"
-FAIL Test removing the end of appended data. assert_equals: Initial buffered range. expected "{ [0.095, 6.548) }" but got "{ [0.000, 6.548) }"
+PASS Test removing all appended data.
+PASS Test removing beginning of appended data.
+FAIL Test removing the middle of appended data. assert_equals: Buffered ranges after remove(). expected "{ [0.095, 0.997) [3.298, 6.548) }" but got "{ [0.095, 0.975) [3.298, 6.548) }"
+FAIL Test removing the end of appended data. assert_equals: Buffered ranges after remove(). expected "{ [0.095, 1.022) }" but got "{ [0.095, 0.995) }"
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-10-22  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Honor MP4 edit lists, bis
+        https://bugs.webkit.org/show_bug.cgi?id=231019
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        This patch takes into consideration the GstSegment attached to a
+        sample to offset the PTS and DTS. This ensures accurate timestamps are
+        obtained for MP4 files containing edit lists (commonly necessary for
+        files containing video with B frames to have PTS starting at zero).
+
+        Before this was implemented, a workaround was in place based on a
+        heuristic (DTS = 0 && PTS > 0 && PTS < 0.1). The workaround is
+        preserved for the sake of content without proper edit lists, but
+        any edit list takes preference.
+
+        The time fudge factor has been modified from 0.083 seconds up to
+        0.100 seconds to accomodate the size of the empty edit in test.mp4
+        used by Web Platform Tests.
+
+        This test fixes improves expectation results and fixes two subtests in
+        imported/w3c/web-platform-tests/media-source/mediasource-remove.html.
+
+        This is a reworked version that avoids using gst_sample_set_buffer()
+        which is not available on GStreamer 1.14, and fixes an issue where
+        frames that would get a negative DTS were not being enqueued properly.
+
+        * Modules/mediasource/MediaSource.cpp:
+        (WebCore::MediaSource::currentTimeFudgeFactor):
+        * platform/graphics/SourceBufferPrivate.h:
+        (WebCore::SourceBufferPrivate::timeFudgeFactor const):
+        * platform/graphics/gstreamer/GStreamerCommon.h:
+        (WebCore::toGstClockTime):
+        * platform/graphics/gstreamer/MediaSampleGStreamer.cpp:
+        (WebCore::MediaSampleGStreamer::MediaSampleGStreamer):
+        * platform/graphics/gstreamer/mse/AppendPipeline.cpp:
+        (WebCore::bufferTimeToStreamTime):
+        (WebCore::AppendPipeline::appsinkNewSample):
+
 2021-10-22  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebCore/Modules/mediasource/MediaSource.cpp

@@ -324 +324 @@
 const MediaTime& MediaSource::currentTimeFudgeFactor()
 {
-    // Allow hasCurrentTime() to be off by as much as the length of two 24fps video frames
-    static NeverDestroyed<MediaTime> fudgeFactor(2002, 24000);
+    // Allow hasCurrentTime() to be off by as much as 100ms.
+    static NeverDestroyed<MediaTime> fudgeFactor(1, 10);
     return fudgeFactor;
 }

trunk/Source/WebCore/platform/graphics/SourceBufferPrivate.h

@@ -151 +151 @@
     // The following method should never be called directly and be overridden instead.
     WEBCORE_EXPORT virtual void append(Vector<unsigned char>&&);
-    virtual MediaTime timeFudgeFactor() const { return {2002, 24000}; }
+    virtual MediaTime timeFudgeFactor() const { return { 1, 10 }; }
     virtual bool isActive() const { return false; }
     virtual bool isSeeking() const { return false; }

trunk/Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h

@@ -81 +81 @@
 inline GstClockTime toGstClockTime(const MediaTime &mediaTime)
 {
+    if (mediaTime.isInvalid())
+        return GST_CLOCK_TIME_NONE;
+    if (mediaTime < MediaTime::zeroTime())
+        return 0;
     return static_cast<GstClockTime>(toGstUnsigned64Time(mediaTime));
 }

trunk/Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp

@@ -73 +73 @@
 
     m_size = gst_buffer_get_size(buffer);
-    m_sample = sample;
+    m_sample = adoptGRef(gst_sample_new(buffer, gst_sample_get_caps(sample.get()), nullptr,
+        gst_sample_get_info(sample.get()) ? gst_structure_copy(gst_sample_get_info(sample.get())) : nullptr));
 
     if (GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT))

trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp

@@ -375 +375 @@
 }
 
+static MediaTime bufferTimeToStreamTime(const GstSegment* segment, GstClockTime bufferTime)
+{
+    if (bufferTime == GST_CLOCK_TIME_NONE)
+        return MediaTime::invalidTime();
+
+    guint64 streamTime;
+    int sign = gst_segment_to_stream_time_full(segment, GST_FORMAT_TIME, bufferTime, &streamTime);
+    if (!sign) {
+        GST_ERROR("Couldn't map buffer time %" GST_TIME_FORMAT " to segment %" GST_PTR_FORMAT, GST_TIME_ARGS(bufferTime), segment);
+        return MediaTime::invalidTime();
+    }
+    return MediaTime(sign * streamTime, GST_SECOND);
+}
+
 void AppendPipeline::appsinkNewSample(const Track& track, GRefPtr<GstSample>&& sample)
 {
     ASSERT(isMainThread());
 
-    if (UNLIKELY(!gst_sample_get_buffer(sample.get()))) {
+    GstBuffer* buffer = gst_sample_get_buffer(sample.get());
+    if (UNLIKELY(!buffer)) {
         GST_WARNING("Received sample without buffer from appsink.");
         return;
     }
 
-    if (!GST_BUFFER_PTS_IS_VALID(gst_sample_get_buffer(sample.get()))) {
+    if (!GST_BUFFER_PTS_IS_VALID(buffer)) {
         // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those.
         GST_DEBUG("Ignoring sample without PTS: %" GST_PTR_FORMAT, gst_sample_get_buffer(sample.get()));
@@ -390 +405 @@
     }
 
+    GstSegment* segment = gst_sample_get_segment(sample.get());
     auto mediaSample = MediaSampleGStreamer::create(WTFMove(sample), track.presentationSize, track.trackId);
+
+    if (segment && (segment->time || segment->start)) {
+        // MP4 has the concept of edit lists, where some buffer time needs to be offsetted, often very slightly,
+        // to get exact timestamps.
+        MediaTime pts = bufferTimeToStreamTime(segment, GST_BUFFER_PTS(buffer));
+        MediaTime dts = bufferTimeToStreamTime(segment, GST_BUFFER_DTS(buffer));
+        GST_TRACE_OBJECT(track.appsinkPad.get(), "Mapped buffer to segment, PTS %" GST_TIME_FORMAT " -> %s DTS %" GST_TIME_FORMAT " -> %s",
+            GST_TIME_ARGS(GST_BUFFER_PTS(buffer)), pts.toString().utf8().data(), GST_TIME_ARGS(GST_BUFFER_DTS(buffer)), dts.toString().utf8().data());
+        mediaSample->setTimestamps(pts, dts);
+    } else if (!GST_BUFFER_DTS(buffer) && GST_BUFFER_PTS(buffer) > 0 && GST_BUFFER_PTS(buffer) <= 100'000'000) {
+        // Because a track presentation time starting at some close to zero, but not exactly zero time can cause unexpected
+        // results for applications, we extend the duration of this first sample to the left so that it starts at zero.
+        // This is relevant for files that should have an edit list but don't, or when using GStreamer < 1.16, where
+        // edit lists are not parsed in push-mode.
+
+        GST_DEBUG("Extending first sample of track '%s' to make it start at PTS=0 %" GST_PTR_FORMAT, track.trackId.string().utf8().data(), buffer);
+        mediaSample->extendToTheBeginning();
+    }
 
     GST_TRACE("append: trackId=%s PTS=%s DTS=%s DUR=%s presentationSize=%.0fx%.0f",
@@ -398 +432 @@
         mediaSample->duration().toString().utf8().data(),
         mediaSample->presentationSize().width(), mediaSample->presentationSize().height());
-
-    // Hack, rework when GStreamer >= 1.16 becomes a requirement:
-    // We're not applying edit lists. GStreamer < 1.16 doesn't emit the correct segments to do so.
-    // GStreamer fix in https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/commit/c2a0da8096009f0f99943f78dc18066965be60f9
-    // Also, in order to apply them we would need to convert the timestamps to stream time, which we're not currently
-    // doing for consistency between GStreamer versions.
-    //
-    // In consequence, the timestamps we're handling here are unedited track time. In track time, the first sample is
-    // guaranteed to have DTS == 0, but in the case of streams with B-frames, often PTS > 0. Edit lists fix this by
-    // offsetting all timestamps by that amount in movie time, but we can't do that if we don't have access to them.
-    // (We could assume the track PTS of the sample with track DTS = 0 is the offset, but we don't have any guarantee
-    // we will get appended that sample first, or ever).
-    //
-    // Because a track presentation time starting at some close to zero, but not exactly zero time can cause unexpected
-    // results for applications, we extend the duration of this first sample to the left so that it starts at zero.
-    if (mediaSample->decodeTime() == MediaTime::zeroTime() && mediaSample->presentationTime() > MediaTime::zeroTime() && mediaSample->presentationTime() <= MediaTime(1, 10)) {
-        GST_DEBUG("Extending first sample to make it start at PTS=0");
-        mediaSample->extendToTheBeginning();
-    }
 
     m_sourceBufferPrivate.didReceiveSample(mediaSample.get());