Bug 233541 - [GStreamer] requestVideoFrameCallback support
Summary: [GStreamer] requestVideoFrameCallback support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-28 03:49 PST by Philippe Normand
Modified: 2021-12-01 10:37 PST (History)
17 users (show)

See Also:


Attachments
Patch (41.89 KB, patch)
2021-11-28 04:03 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (41.90 KB, patch)
2021-11-30 07:58 PST, Philippe Normand
calvaris: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2021-11-28 03:49:38 PST
SSIA
Comment 1 Philippe Normand 2021-11-28 04:03:52 PST
Created attachment 445238 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2021-11-29 09:15:30 PST
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).
Comment 3 Philippe Normand 2021-11-29 10:23:44 PST
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 :)
Comment 4 Xabier Rodríguez Calvar 2021-11-30 02:21:55 PST
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:
Comment 5 Philippe Normand 2021-11-30 07:58:35 PST
Created attachment 445418 [details]
Patch
Comment 6 Xabier Rodríguez Calvar 2021-12-01 01:15:46 PST
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.
Comment 7 Philippe Normand 2021-12-01 10:36:48 PST
Committed r286369 (244728@main): <https://commits.webkit.org/244728@main>
Comment 8 Radar WebKit Bug Importer 2021-12-01 10:37:27 PST
<rdar://problem/85929030>