SSIA
Created attachment 445238 [details] Patch
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 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 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:
Created attachment 445418 [details] Patch
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.
Committed r286369 (244728@main): <https://commits.webkit.org/244728@main>
<rdar://problem/85929030>