Bug 200584 – [GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer deocoders

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

[Thibault Saunier]

[GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer deocoders

[Thibault Saunier]

Created attachment 375940 [details]
Patch

[Philippe Normand]

Comment on attachment 375940 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Listenning to parsers warnings and error messages (synhronously so that we react
> +        right away) and requesting keyframes from the peer.
> +
> +        Also simplify the decoder code by trying to make decoding happen
> +        in one single pass, also hidding away GStreamer threading and allowing
> +        us to react as soon as the decoder/parser fails.

Fix spelling please :)

> Source/WebCore/ChangeLog:23
> +        * Scripts/webkitpy/style/checker.py:
> +        * gstreamer/jhbuild.modules:
> +        * gstreamer/patches/gst-plugins-bad-0001-h264parse-Post-a-WARNING-when-data-is-broken.patch: Added.

This seems copy/pasted from the Tools/ChangeLog, please remove

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:103
> +        auto sinkpad = gst_element_get_static_pad(capsfilter, "sink");

Where is this unreffed?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:115
> +                GError* err = nullptr;

Can you use a GUniqueOutPtr for this please?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:117
> +                switch (GST_MESSAGE_TYPE(message)) {
> +                case GST_MESSAGE_WARNING: {

As the code is the same for warnings and errors, perhaps there's way to simplify this with a if-test instead of a switch?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:343
> +    GstElement* m_sink;
>      GstElement* m_src;

Would it be doable to make these GRefPtr<GstElement> ?

> Tools/ChangeLog:11
> +        Listenning to parsers warnings and error messages (synhronously so that we react
> +        right away) and requesting keyframes from the peer.
> +
> +        Also simplify the decoder code by trying to make decoding happen
> +        in one single pass, also hidding away GStreamer threading and allowing
> +        us to react as soon as the decoder/parser fails.

No need to copy this from WebCore/ChangeLog, you can leave it empty.

> Tools/ChangeLog:19
> +        * platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:
> +        (WebCore::GStreamerVideoDecoder::GStreamerVideoDecoder):
> +        (WebCore::GStreamerVideoDecoder::pullSample):
> +        (WebCore::H264Decoder::H264Decoder):
> +        * platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:

This seems copy/pasted from WebCore/ChangeLog, please remove.

[Thibault Saunier]

Created attachment 376062 [details]
Patch

[Thibault Saunier]

(In reply to Philippe Normand from comment #2)
> Comment on attachment 375940 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375940&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Listenning to parsers warnings and error messages (synhronously so that we react
> > +        right away) and requesting keyframes from the peer.
> > +
> > +        Also simplify the decoder code by trying to make decoding happen
> > +        in one single pass, also hidding away GStreamer threading and allowing
> > +        us to react as soon as the decoder/parser fails.

Done, sorry about that. 

 
> > Source/WebCore/ChangeLog:23
> > +        * Scripts/webkitpy/style/checker.py:
> > +        * gstreamer/jhbuild.modules:
> > +        * gstreamer/patches/gst-plugins-bad-0001-h264parse-Post-a-WARNING-when-data-is-broken.patch: Added.
> 
> This seems copy/pasted from the Tools/ChangeLog, please remove
> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:103
> > +        auto sinkpad = gst_element_get_static_pad(capsfilter, "sink");
> 
> Where is this unreffed?

Using adoptGRef() now.
 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:115
> > +                GError* err = nullptr;
> 
> Can you use a GUniqueOutPtr for this please?

Done.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:117
> > +                switch (GST_MESSAGE_TYPE(message)) {
> > +                case GST_MESSAGE_WARNING: {
> 
> As the code is the same for warnings and errors, perhaps there's way to
> simplify this with a if-test instead of a switch?

Done.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:343
> > +    GstElement* m_sink;
> >      GstElement* m_src;
> 
> Would it be doable to make these GRefPtr<GstElement> ?

It could but as it is own by the pipeline I am not sure what it would bring to the table?
 
> > Tools/ChangeLog:11
> > +        Listenning to parsers warnings and error messages (synhronously so that we react
> > +        right away) and requesting keyframes from the peer.
> > +
> > +        Also simplify the decoder code by trying to make decoding happen
> > +        in one single pass, also hidding away GStreamer threading and allowing
> > +        us to react as soon as the decoder/parser fails.
> 
> No need to copy this from WebCore/ChangeLog, you can leave it empty.

Done, I just rewrote the whole thing without using `prepare-changelog -g` I  have the impression is not quite right with that (or I do not understand how it is supposed to work!).

[WebKit Commit Bot]

Comment on attachment 376062 [details]
Patch

Clearing flags on attachment: 376062

Committed r248530: <https://trac.webkit.org/changeset/248530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54213369>

[Xabier Rodríguez Calvar]

Comment on attachment 376062 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:109
> +            caps = gst_caps_new_simple(Caps(), "parsed", G_TYPE_BOOLEAN, TRUE, nullptr);

I think we're leaking here and in the else because we should be adopting. Am I missing anything?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:255
> +        auto buffer = gst_sample_get_buffer(sample);

Aren't we leaking the buffer/sample here?

[Thibault Saunier]

(In reply to Xabier Rodríguez Calvar from comment #8)
> Comment on attachment 376062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376062&action=review
> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:109
> > +            caps = gst_caps_new_simple(Caps(), "parsed", G_TYPE_BOOLEAN, TRUE, nullptr);

Oops, indeed, the caps are leaked.

> I think we're leaking here and in the else because we should be adopting. Am
> I missing anything?
> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:255
> > +        auto buffer = gst_sample_get_buffer(sample);
> 
> Aren't we leaking the buffer/sample here?

No, get_buffer returns "transfer none" and the sample ownership is passed to the frame later in the code.

[Philippe Normand]

Comment on attachment 376062 [details]
Patch

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

>> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:109
>> +            caps = gst_caps_new_simple(Caps(), "parsed", G_TYPE_BOOLEAN, TRUE, nullptr);
> 
> I think we're leaking here and in the else because we should be adopting. Am I missing anything?

Looks like a leak yes.

>> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:255
>> +        auto buffer = gst_sample_get_buffer(sample);
> 
> Aren't we leaking the buffer/sample here?

The buffer is transfer-none. The sample is adopted in GStreamerVideoFrameLibWebRTC(). So I think we're fine there?

[Xabier Rodríguez Calvar]

Comment on attachment 376062 [details]
Patch

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

>>>> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:255
>>>> +        auto buffer = gst_sample_get_buffer(sample);
>>> 
>>> Aren't we leaking the buffer/sample here?
>> 
>> No, get_buffer returns "transfer none" and the sample ownership is passed to the frame later in the code.
> 
> The buffer is transfer-none. The sample is adopted in GStreamerVideoFrameLibWebRTC(). So I think we're fine there?

Yes, we are.