Bug 190035 – [GStreamer] Fix EncodedImage timestamps to match what libWebRTC expects

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

[GStreamer] Fix EncodedImage timestamps to match what libWebRTC expects

[Thibault Saunier]

Created attachment 350957 [details]
Patch

[Alejandro G. Castro]

Comment on attachment 350957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350957&action=review

Great catch, tricky problem, LGTM with some small changes.

> Source/WebCore/ChangeLog:12
> +        all the GStreamer processing pipelines as the WebRTC object basically wrap the "same"

Nit, wrap -> wraps

> Source/WebCore/ChangeLog:26
> +        because of that issue.

Nit, is this a leftover?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:424
> +    std::map<GstClockTime, RtpTimestamps> m_ptsRtpTimestampMap;

We usually use HashMap type defined in WTF, is there any feature in the std::maps that we want to use?

[Thibault Saunier]

Created attachment 352612 [details]
Patch

[Thibault Saunier]

Created attachment 353868 [details]
Patch

[Philippe Normand]

Comment on attachment 353868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353868&action=review

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:70
> +        , m_adapter(adoptGRef(gst_adapter_new()))

Hum. I think this needs a GRefPtrGStreamer template specialization to avoid leaks.

[Philippe Normand]

Comment on attachment 353868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353868&action=review

>> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:70
>> +        , m_adapter(adoptGRef(gst_adapter_new()))
> 
> Hum. I think this needs a GRefPtrGStreamer template specialization to avoid leaks.

Ah no, it's a normal GObject. Nevermind :)

[WebKit Commit Bot]

Comment on attachment 353868 [details]
Patch

Clearing flags on attachment: 353868

Committed r237819: <https://trac.webkit.org/changeset/237819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45813371>

[Xabier Rodríguez Calvar]

Comment on attachment 353868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353868&action=review

I'm late, but if there's any follow up, we can fix this couple of things.

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:242
> +        m_bufferMapLock.lock();
> +        // Make sure that the frame.timestamp == previsouly input_frame._timeStamp
> +        // as it is required by the VideoDecoder baseclass.
> +        auto timestamps = m_dtsPtsMap[GST_BUFFER_PTS(buffer)];
> +        m_dtsPtsMap.erase(GST_BUFFER_PTS(buffer));
> +        m_bufferMapLock.unlock();

Nit: I would have used a Locker here by either using unlockEarly or just using { } to create scope.

Besides, previsouly -> previously.

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:266
> +                uint flags = GST_BUFFER_FLAGS(buffer);

unsigned