[webkit-reviews] review denied: [Bug 200584] [GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer deocoders : [Attachment 375940] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Aug 10 04:21:03 PDT 2019


Philippe Normand <pnormand at igalia.com> has denied Thibault Saunier
<tsaunier at gnome.org>'s request for review:
Bug 200584: [GStreamer][WebRTC] Handle broken data in the libwebrtc GStreamer
deocoders
https://bugs.webkit.org/show_bug.cgi?id=200584

Attachment 375940: Patch

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




--- Comment #2 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 375940
  --> https://bugs.webkit.org/attachment.cgi?id=375940
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-br
oken.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.


More information about the webkit-reviews mailing list