[webkit-reviews] review granted: [Bug 222451] REGRESSION(r273309) [GStreamer] webrtc/captureCanvas-webrtc-software-h264-baseline.html is flaky crashing inside libwebrtc : [Attachment 422074] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 222451: REGRESSION(r273309) [GStreamer]
webrtc/captureCanvas-webrtc-software-h264-baseline.html is flaky crashing
inside libwebrtc
https://bugs.webkit.org/show_bug.cgi?id=222451

Attachment 422074: Patch

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




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 422074
  --> https://bugs.webkit.org/attachment.cgi?id=422074
Patch

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

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:
34
> +	   return framebuffer->takeSample();

WARN_UNUSED_RETURN GRefPtr<GstSample>&&
GStreamerSampleFromLibWebRTCVideoFrame(const webrtc::VideoFrame& frame)

You seem to want a cascade of no copies and here there is one, explanation
below.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:
38
> +    auto b = frame.video_frame_buffer()->ToI420();
> +    auto* i420Buffer = b.release();

You can do this in the same call, can't you?

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:
46
> +    gsize offsets[3] = {

size_t?

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:
49
> +	   static_cast<gsize>(i420Buffer->StrideY() * height +
i420Buffer->StrideU() * ((height +1) / 2))

height + 1

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:
55
> +	   const_cast<gpointer>(reinterpret_cast<const
void*>(i420Buffer->DataY())),
> +	   info.size, 0, info.size, i420Buffer, [](gpointer b) {

these lines can be one. And b -> buffer

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:
56
> +	       reinterpret_cast<webrtc::I420Buffer*>(b)->Release();

Rant: ain't it confusing they use Release here and then they wrap this in smart
pointers where you have release() ?

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.h:46
> +    GRefPtr<GstSample> takeSample() { return WTFMove(m_sample); }

For the name and the code of what you're doing I assume the intention of this
code is returning the sample we got without any copy and "thus" releasing what
we have here. But it is not happening, there is an implicit copy here: what is
happening here would be "equivalent" to, if I am not mistaken:

GRefPtr<GstSample> takeSample() {
    GRefPtr sample(WTFMove(m_sample)); // We create an object by moving what we
have.
    return sample; // Copy elision does the magic and avoids the copy.
}

We're doing at least one copy of the pointer here, which I guess is not the
intention and it works because of the "equivalence" I just explained.

For you to avoid this, you would need to do something like:

GRefPtr<GstSample>&& takeSample() { return WTFMove(m_sample); }

But this has a huge problem and it is that if you call this with the intention
of just releasing the internal frame without assigning it anywhere like just:

framebuffer->takeFrame();

it will do absolutely nothing and your value will be still there after the
call.

But worry not, we can has WARN_UNUSED_RETURN GRefPtr<GstSample>&& takeSample()
{ return WTFMove(m_sample); }