Bug 190684 – [GStreamer][WebRTC] Implement black frame generation

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

[Thibault Saunier]

[GStreamer][WebRTC] Implement black frame generation

[Thibault Saunier]

Created attachment 352624 [details]
Patch

[Thibault Saunier]

Created attachment 353871 [details]
Patch

[Thibault Saunier]

Created attachment 353884 [details]
Patch

[Xabier Rodríguez Calvar]

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;
    }

[Xabier Rodríguez Calvar]

(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.

[Thibault Saunier]

(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.

[Thibault Saunier]

Created attachment 353965 [details]
Patch for landing

[WebKit Commit Bot]

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 [e-mail]...
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

[Thibault Saunier]

Created attachment 353966 [details]
Patch for landing

[WebKit Commit Bot]

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

[Thibault Saunier]

Created attachment 353967 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 353967 [details]
Patch for landing

Clearing flags on attachment: 353967

Committed r237861: <https://trac.webkit.org/changeset/237861>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45841710>