Bug 189921 – [WPE][GTK][WebRTC] Fixup VP8 encoding support

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

[Thibault Saunier]

[WPE][GTK][WebRTC] Fixup VP8 encoding support

[Thibault Saunier]

Created attachment 350658 [details]
Patch

[Philippe Normand]

Comment on attachment 350658 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:380
>          fragmentationInfo->fragmentationTimeDiff[0] = 0;
> +        gst_buffer_unmap(buffer, &map);

You could use the GstMappedBuffer thing here maybe?

[Thibault Saunier]

Created attachment 350746 [details]
Patch

[Philippe Normand]

Comment on attachment 350746 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        + And do not raise a GStreamer error when GStreanerVideoEnclder::OnEncodedImage

typo Enclder

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:84
> +        SetRestrictionCaps(WTFMove(caps));

Are all the SetRestrictionCaps call sites OK now?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:255
> +            GST_ERROR_OBJECT(m_pipeline.get(), "Encode callback failed: %d", result.error);
>  
>          return GST_FLOW_OK;

Is it OK to return FLOW_OK also when an error is raised?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:370
> +        memcpy(encodedImage->_buffer, map.data(), map.size());

I know we had this before but would there be a way to avoid this?

[Thibault Saunier]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 350746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350746&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        + And do not raise a GStreamer error when GStreanerVideoEnclder::OnEncodedImage
> 
> typo Enclder

Fixed.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:84
> > +        SetRestrictionCaps(WTFMove(caps));
> 
> Are all the SetRestrictionCaps call sites OK now?

It is the only call yes.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:255
> > +            GST_ERROR_OBJECT(m_pipeline.get(), "Encode callback failed: %d", result.error);
> >  
> >          return GST_FLOW_OK;
> 
> Is it OK to return FLOW_OK also when an error is raised?

It is not raising an error anymore, just a "small" runtime `GST_ERROR`, the only 2 return values for `webrtc::EncodedImageCallback::Result` are:
  * OK
  * ERROR_SEND_FAILED

so basically we should not stop processing when `ERROR_SEND_FAILED` happens, so no reason to post an error on the bus.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:370
> > +        memcpy(encodedImage->_buffer, map.data(), map.size());
> 
> I know we had this before but would there be a way to avoid this?

There might be a way but I can't see any encoder implementation avoiding this memcpy, it is an encoded frame so it shouldn't be a huge deal in practice.

[Xabier Rodríguez Calvar]

Comment on attachment 350746 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        + And do not raise a GStreamer error when GStreanerVideoEnclder::OnEncodedImage
> 
> typo Enclder

and GStreaner :)

[Thibault Saunier]

Created attachment 350748 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350748 [details]
Patch

Clearing flags on attachment: 350748

Committed r236466: <https://trac.webkit.org/changeset/236466>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44768397>