Bug 190035

Summary: [GStreamer] Fix EncodedImage timestamps to match what libWebRTC expects
Product: WebKit Reporter: Thibault Saunier <tsaunier>
Component: New BugsAssignee: Thibault Saunier <tsaunier>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, calvaris, commit-queue, pnormand, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 190678    
Bug Blocks: 187064, 190682    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Thibault Saunier
Reported 2018-09-27 06:13:15 PDT
[GStreamer] Fix EncodedImage timestamps to match what libWebRTC expects
Attachments
Patch (4.40 KB, patch)
2018-09-27 06:18 PDT, Thibault Saunier 		no flags
DetailsFormatted DiffDiff
Patch (14.25 KB, patch)
2018-10-17 12:39 PDT, Thibault Saunier 		no flags
DetailsFormatted DiffDiff
Patch (14.25 KB, patch)
2018-11-05 09:19 PST, Thibault Saunier 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Thibault Saunier
Comment 1 2018-09-27 06:18:37 PDT
Created attachment 350957 [details] Patch
Alejandro G. Castro
Comment 2 2018-09-27 07:55:51 PDT
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
Comment 3 2018-10-17 12:39:14 PDT
Created attachment 352612 [details] Patch
Thibault Saunier
Comment 4 2018-11-05 09:19:32 PST
Created attachment 353868 [details] Patch
Philippe Normand
Comment 5 2018-11-05 10:40:45 PST
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 6 2018-11-05 10:43:30 PST
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 7 2018-11-05 11:15:22 PST
Comment on attachment 353868 [details] Patch Clearing flags on attachment: 353868 Committed r237819: <https://trac.webkit.org/changeset/237819>
WebKit Commit Bot
Comment 8 2018-11-05 11:15:23 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 9 2018-11-05 11:16:24 PST
<rdar://problem/45813371>
Xabier Rodríguez Calvar
Comment 10 2018-11-05 23:15:41 PST
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
Note You need to log in before you can comment on or make changes to this bug.