Bug 200584 - [GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer deocoders
Summary: [GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer deocoders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thibault Saunier
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-09 12:07 PDT by Thibault Saunier
Modified: 2019-08-26 23:42 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.86 KB, patch)
2019-08-09 12:09 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (16.02 KB, patch)
2019-08-12 08:01 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thibault Saunier 2019-08-09 12:07:33 PDT
[GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer deocoders
Comment 1 Thibault Saunier 2019-08-09 12:09:26 PDT
Created attachment 375940 [details]
Patch
Comment 2 Philippe Normand 2019-08-10 04:21:03 PDT
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.
Comment 3 Thibault Saunier 2019-08-12 08:01:34 PDT
Created attachment 376062 [details]
Patch
Comment 4 Thibault Saunier 2019-08-12 08:01:46 PDT
(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!).
Comment 5 WebKit Commit Bot 2019-08-12 09:56:08 PDT
Comment on attachment 376062 [details]
Patch

Clearing flags on attachment: 376062

Committed r248530: <https://trac.webkit.org/changeset/248530>
Comment 6 WebKit Commit Bot 2019-08-12 09:56:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-08-12 09:57:17 PDT
<rdar://problem/54213369>
Comment 8 Xabier Rodríguez Calvar 2019-08-26 06:35:20 PDT
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?
Comment 9 Thibault Saunier 2019-08-26 06:52:32 PDT
(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.
Comment 10 Philippe Normand 2019-08-26 06:56:05 PDT
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?
Comment 11 Xabier Rodríguez Calvar 2019-08-26 23:42:32 PDT
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.