Summary: | [GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer deocoders | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Thibault Saunier <tsaunier> | ||||||
Component: | New Bugs | Assignee: | Thibault Saunier <tsaunier> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | calvaris, commit-queue, pnormand, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Thibault Saunier
2019-08-09 12:07:33 PDT
Created attachment 375940 [details]
Patch
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. Created attachment 376062 [details]
Patch
(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 on attachment 376062 [details] Patch Clearing flags on attachment: 376062 Committed r248530: <https://trac.webkit.org/changeset/248530> All reviewed patches have been landed. Closing bug. 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? (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 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 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. |