Bug 233541 – [GStreamer] requestVideoFrameCallback support

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

SSIA

[Philippe Normand]

Created attachment 445238 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 445238 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.h:38
> +    static Ref<MediaSampleGStreamer> create(GRefPtr<GstSample>&& sample, const FloatSize& presentationSize, const AtomString& trackId, VideoRotation videoRotation = VideoRotation::None, bool videoMirrored = false, std::optional<VideoSampleMetadata> metadata = { })

I think it should be a std::optional&& and passing a std::nullopt as default;

> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.h:40
> +        return adoptRef(*new MediaSampleGStreamer(WTFMove(sample), presentationSize, trackId, videoRotation, videoMirrored, metadata));

move here

> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.h:44
> +    static Ref<MediaSampleGStreamer> createImageSample(PixelBuffer&&, const IntSize& destinationSize = { }, double frameRate = 1, VideoRotation videoRotation = VideoRotation::None, bool videoMirrored = false, std::optional<VideoSampleMetadata> metadata = { });

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.h:68
> +    MediaSampleGStreamer(GRefPtr<GstSample>&&, const FloatSize& presentationSize, const AtomString& trackId, VideoRotation = VideoRotation::None, bool videoMirrored = false, std::optional<VideoSampleMetadata> metadata = { });

Ditto

> Source/WebCore/platform/graphics/gstreamer/VideoFrameMetadataGStreamer.cpp:93
> +                auto [buf, copyMeta] = ensureVideoFrameMetadata(buffer);

Aren't these vars unused?

> Source/WebCore/platform/graphics/gstreamer/VideoFrameMetadataGStreamer.cpp:122
> +    GUniquePtr<char> elementNameForSinkPad(gst_element_get_name(element));

You better spare the smart ptr here. It's mostly useless.

> Source/WebCore/platform/graphics/gstreamer/VideoFrameMetadataGStreamer.cpp:131
> +    GUniquePtr<char> elementNameForSrcPad(gst_element_get_name(element));

Ditto.

> Source/WebCore/platform/mediastream/gstreamer/MockRealtimeVideoSourceGStreamer.cpp:157
> +    VideoSampleMetadata metadata;
> +    metadata.captureTime = MonotonicTime::now().secondsSinceEpoch();
> +    auto sample = MediaSampleGStreamer::createImageSample(WTFMove(*pixelBuffer), size(), frameRate(), sampleRotation(), false, metadata);

You might want to emplace the optional here and move it down.

> Source/WebCore/platform/mediastream/libwebrtc/gstreamer/RealtimeIncomingVideoSourceLibWebRTC.cpp:64
> +    auto metadata = metadataFromVideoFrame(frame);
> +    callOnMainThread([protectedThis = Ref { *this }, frame, metadata] {
>          auto gstSample = GStreamerSampleFromLibWebRTCVideoFrame(frame);
> -        auto sample = MediaSampleGStreamer::create(WTFMove(gstSample), { }, { });
> +        auto sample = MediaSampleGStreamer::create(WTFMove(gstSample), { }, { }, static_cast<MediaSample::VideoRotation>(frame.rotation()), false, metadata);

You can probably create the optional, move the optional into the lambda and move again when calling the constructor (for this you'll need to make the lambda mutable).

[Philippe Normand]

Comment on attachment 445238 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.h:38
>> +    static Ref<MediaSampleGStreamer> create(GRefPtr<GstSample>&& sample, const FloatSize& presentationSize, const AtomString& trackId, VideoRotation videoRotation = VideoRotation::None, bool videoMirrored = false, std::optional<VideoSampleMetadata> metadata = { })
> 
> I think it should be a std::optional&& and passing a std::nullopt as default;

I wonder what's the difference :) Are we really avoiding an allocation in this scenario? Seems to add more confusion than anything else: https://ddanilov.me/the-state-of-std-optional-after-move/

>> Source/WebCore/platform/graphics/gstreamer/VideoFrameMetadataGStreamer.cpp:93
>> +                auto [buf, copyMeta] = ensureVideoFrameMetadata(buffer);
> 
> Aren't these vars unused?

buf yes, copyMeta is used just below :)

[Xabier Rodríguez Calvar]

Comment on attachment 445238 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.h:38
>>> +    static Ref<MediaSampleGStreamer> create(GRefPtr<GstSample>&& sample, const FloatSize& presentationSize, const AtomString& trackId, VideoRotation videoRotation = VideoRotation::None, bool videoMirrored = false, std::optional<VideoSampleMetadata> metadata = { })
>> 
>> I think it should be a std::optional&& and passing a std::nullopt as default;
> 
> I wonder what's the difference :) Are we really avoiding an allocation in this scenario? Seems to add more confusion than anything else: https://ddanilov.me/the-state-of-std-optional-after-move/

Yes, we should be avoiding an allocation of the std::optional object. We should do it the same way we do for the other parameters, like sample, PixelBuffer below, etc.

>>> Source/WebCore/platform/graphics/gstreamer/VideoFrameMetadataGStreamer.cpp:93
>>> +                auto [buf, copyMeta] = ensureVideoFrameMetadata(buffer);
>> 
>> Aren't these vars unused?
> 
> buf yes, copyMeta is used just below :)

:facepalm:

[Philippe Normand]

Created attachment 445418 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 445418 [details]
Patch

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

There are a couple style issues from the bot as well.

> Source/WebCore/platform/graphics/gstreamer/VideoFrameMetadataGStreamer.cpp:122
> +    auto* elementNameForSinkPad = gst_element_get_name(element);

You could even inline this call as parameter of add_probe and save a var.

> Source/WebCore/platform/graphics/gstreamer/VideoFrameMetadataGStreamer.cpp:131
> +    auto* elementNameForSrcPad = gst_element_get_name(element);

ditto.

[Philippe Normand]

Committed r286369 (244728@main): <https://commits.webkit.org/244728@main>

[Radar WebKit Bug Importer]

<rdar://problem/85929030>