Bug 190684

Created attachment 352624 [details] Patch

Created attachment 353871 [details] Patch

Created attachment 353884 [details] Patch

Comment on attachment 353884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353884&action=review > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceLibWebRTC.cpp:90 > + GRefPtr<GstCaps> caps = gst_video_info_to_caps(&info); I think we are leaking the caps here. They should be adopted. > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceLibWebRTC.cpp:92 > + GstMappedBuffer map(buffer.get(), GST_MAP_WRITE); Bonus points! Could you please write this helper in GStreamerCommon.h? GstMappedBuffer(const GRefPtr<GstBuffer>& buffer, GstMapFlags flags) : GstMappedBuffer(buffer.get(), flags) { } Then you don't have to pass buffer.get(). More bonus points! I see it would be interesting to also write an ASSERT(GST_IS_BUFFER(m_buffer)); at: ~GstMappedBuffer() { if (m_isValid) { ASSERT(GST_IS_BUFFER(m_buffer)); gst_buffer_unmap(m_buffer, &m_info); } m_isValid = false; }

(In reply to Xabier Rodríguez Calvar from comment #4) > More bonus points! > > I see it would be interesting to also write an > ASSERT(GST_IS_BUFFER(m_buffer)); at: > > ~GstMappedBuffer() > { > if (m_isValid) { > ASSERT(GST_IS_BUFFER(m_buffer)); > gst_buffer_unmap(m_buffer, &m_info); > > } > m_isValid = false; > } And maybe at the first constructor as well. There are g_checks but for us it is safer to have ASSERT.

(In reply to Xabier Rodríguez Calvar from comment #4) > Comment on attachment 353884 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353884&action=review > > > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceLibWebRTC.cpp:90 > > + GRefPtr<GstCaps> caps = gst_video_info_to_caps(&info); > > I think we are leaking the caps here. They should be adopted. Fixed. > > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceLibWebRTC.cpp:92 > > + GstMappedBuffer map(buffer.get(), GST_MAP_WRITE); > > Bonus points! > > Could you please write this helper in GStreamerCommon.h? > > GstMappedBuffer(const GRefPtr<GstBuffer>& buffer, GstMapFlags flags) > : GstMappedBuffer(buffer.get(), flags) { } Well, `.get()` is not so cumbersome and I am not sure we have found a pattern that would tell us it is necessary to add tbh. > Then you don't have to pass buffer.get(). > > More bonus points! > > I see it would be interesting to also write an > ASSERT(GST_IS_BUFFER(m_buffer)); at: > > ~GstMappedBuffer() > { > if (m_isValid) { > ASSERT(GST_IS_BUFFER(m_buffer)); > gst_buffer_unmap(m_buffer, &m_info); > > } > m_isValid = false; > } gst_buffer_unmap()should already do a g_return_if_fail, and it is the right place for this check fmopv.

Created attachment 353965 [details] Patch for landing

Comment on attachment 353965 [details] Patch for landing Rejecting attachment 353965 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 353965, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=353965&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=190684&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 353965 from bug 190684. Fetching: https://bugs.webkit.org/attachment.cgi?id=353965 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 15 diffs from patch file(s). patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp Hunk #1 FAILED at 154. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp.rej patching file Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp Hunk #1 FAILED at 69. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp.rej patching file Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.h Hunk #1 FAILED at 32. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.h.rej patching file Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceLibWebRTC.cpp patching file Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceLibWebRTC.h patching file Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp Hunk #1 succeeded at 49 with fuzz 2 (offset 5 lines). Hunk #2 FAILED at 177. Hunk #3 FAILED at 239. Hunk #4 FAILED at 280. 3 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp.rej patching file Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoder.cpp patching file Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp Hunk #1 FAILED at 40. Hunk #2 FAILED at 56. Hunk #3 succeeded at 87 (offset 4 lines). Hunk #4 succeeded at 135 (offset 4 lines). Hunk #5 succeeded at 150 (offset 4 lines). Hunk #6 FAILED at 195. Hunk #7 FAILED at 219. Hunk #8 succeeded at 287 with fuzz 2 (offset 35 lines). Hunk #9 FAILED at 321. Hunk #10 FAILED at 365. Hunk #11 succeeded at 408 (offset 31 lines). Hunk #12 FAILED at 435. Hunk #13 succeeded at 459 (offset 35 lines). Hunk #14 FAILED at 550. 8 out of 14 hunks FAILED -- saving rejects to file Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp.rej patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/flatpak/flatpakutils.py patching file Tools/flatpak/org.webkit.WebKit.yaml Hunk #1 FAILED at 173. Hunk #2 FAILED at 192. 2 out of 2 hunks FAILED -- saving rejects to file Tools/flatpak/org.webkit.WebKit.yaml.rej patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/gtk/TestExpectations patching file LayoutTests/platform/wpe/TestExpectations Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/9878305

Created attachment 353966 [details] Patch for landing

Comment on attachment 353966 [details] Patch for landing Rejecting attachment 353966 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 353966, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/9878370

Created attachment 353967 [details] Patch for landing

Comment on attachment 353967 [details] Patch for landing Clearing flags on attachment: 353967 Committed r237861: <https://trac.webkit.org/changeset/237861>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45841710>