[webkit-reviews] review granted: [Bug 190684] [GStreamer][WebRTC] Implement black frame generation : [Attachment 353884] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Thibault Saunier
<tsaunier at gnome.org>'s request for review:
Bug 190684: [GStreamer][WebRTC] Implement black frame generation
https://bugs.webkit.org/show_bug.cgi?id=190684

Attachment 353884: Patch

https://bugs.webkit.org/attachment.cgi?id=353884&action=review




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 353884
  --> https://bugs.webkit.org/attachment.cgi?id=353884
Patch

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

>
Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingVideoSourceLibWeb
RTC.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/RealtimeOutgoingVideoSourceLibWeb
RTC.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;
    }