[GStreamer][WebRTC] Implement GstMappedRtpBuffer by philn · Pull Request #9373 · WebKit/WebKit

philn · 2023-01-31T10:21:26Z

`7878d72`

[GStreamer][WebRTC] Implement GstMappedRtpBuffer
https://bugs.webkit.org/show_bug.cgi?id=251428

Reviewed by Xabier Rodriguez-Calvar.

With this new RAII object for mapping RTP buffers we don't need to manually unmap anymore.

Canonical link: https://commits.webkit.org/261074@main

0ffe5ca

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-31T10:21:42Z

EWS run on previous version of this PR (hash 793f937)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-31T11:05:15Z

EWS run on previous version of this PR (hash 23e2bb8)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

calvaris

If this implementation is the same as the GstMappedBuffer with the only difference of map/unmap functions, I think we should do some templating here and optionally pass the map/unmap functions. Then, if you want, you can create the GstRTPMappedBuffer with a using.

philn · 2023-02-01T09:16:40Z

If this implementation is the same as the GstMappedBuffer with the only difference of map/unmap functions, I think we should do some templating here and optionally pass the map/unmap functions. Then, if you want, you can create the GstRTPMappedBuffer with a using.

The public API differs though.

calvaris · 2023-02-02T07:39:40Z

In what, other than the type?

philn · 2023-02-02T08:31:52Z

In what, other than the type?

GstMappedBuffer has ::size() and ::createVector()

Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.h

calvaris · 2023-02-03T07:09:50Z

GstMappedBuffer has ::size() and ::createVector()

I still think it is worth trying. It does not hurt to have them. You need four template parameters, but nothing outstanding. If you want, you can avoid using default parameters and create a GstBufferBaseMappedBuffer with the four template parameters (GstBuffer, GstMapInfo (which also seems to differ for RTP), map and unmap). Then you create GstBufferMap with a using.

We would be reducing a lot the code duplication.

philn · 2023-02-03T09:21:07Z

GstMappedBuffer has ::size() and ::createVector()

I still think it is worth trying. It does not hurt to have them. You need four template parameters, but nothing outstanding. If you want, you can avoid using default parameters and create a GstBufferBaseMappedBuffer with the four template parameters (GstBuffer, GstMapInfo (which also seems to differ for RTP), map and unmap). Then you create GstBufferMap with a using.

We would be reducing a lot the code duplication.

Yes and I tried this already :) ::createVector() doesn't make much sense for RTP buffers though.

calvaris · 2023-02-07T07:11:36Z

Yes and I tried this already :) ::createVector() doesn't make much sense for RTP buffers though.

You don't need to use all the API. I am very reluctant to create something that is almost equal in code to something that can change a couple of types and functions. That's what templates are for, right?

philn · 2023-02-07T08:33:45Z

Let's revisit this PR when we have more than one usage of gst_rtp_buffer_map().

calvaris · 2023-02-08T07:29:09Z

For me, one use is enough and templating is the way to go ☺️

webkit-early-warning-system · 2023-03-01T17:33:50Z

EWS run on previous version of this PR (hash d761e31)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-03-02T12:21:32Z

Starting EWS tests for 0ffe5ca. Live statuses available at the PR page, #9373

webkit-early-warning-system · 2023-03-02T12:21:32Z

EWS run on current version of this PR (hash 0ffe5ca)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

calvaris

B E A U T I F U L !

https://bugs.webkit.org/show_bug.cgi?id=251428 Reviewed by Xabier Rodriguez-Calvar. With this new RAII object for mapping RTP buffers we don't need to manually unmap anymore. Canonical link: https://commits.webkit.org/261074@main

webkit-commit-queue · 2023-03-02T17:04:56Z

Committed 261074@main (7878d72): https://commits.webkit.org/261074@main

Reviewed commits have been landed. Closing PR #9373 and removing active labels.

philn self-assigned this Jan 31, 2023

philn added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label Jan 31, 2023

philn requested a review from calvaris January 31, 2023 10:21

philn marked this pull request as draft January 31, 2023 10:54

philn force-pushed the eng/251428 branch from 793f937 to 23e2bb8 Compare January 31, 2023 11:04

philn marked this pull request as ready for review January 31, 2023 11:05

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 31, 2023

calvaris requested changes Feb 1, 2023

View reviewed changes

shivamidow reviewed Feb 3, 2023

View reviewed changes

Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.h Outdated Show resolved Hide resolved

philn marked this pull request as draft February 7, 2023 08:34

philn removed the merging-blocked Applied to prevent a change from being merged label Mar 1, 2023

philn force-pushed the eng/251428 branch from 23e2bb8 to d761e31 Compare March 1, 2023 17:33

philn requested a review from calvaris March 1, 2023 17:33

philn marked this pull request as ready for review March 1, 2023 17:34

philn force-pushed the eng/251428 branch from d761e31 to 0ffe5ca Compare March 2, 2023 12:21

calvaris approved these changes Mar 2, 2023

View reviewed changes

philn added the merge-queue Applied to send a pull request to merge-queue label Mar 2, 2023

[GStreamer][WebRTC] Implement GstMappedRtpBuffer

7878d72

https://bugs.webkit.org/show_bug.cgi?id=251428 Reviewed by Xabier Rodriguez-Calvar. With this new RAII object for mapping RTP buffers we don't need to manually unmap anymore. Canonical link: https://commits.webkit.org/261074@main

webkit-early-warning-system force-pushed the eng/251428 branch from 0ffe5ca to 7878d72 Compare March 2, 2023 17:04

webkit-early-warning-system merged commit 7878d72 into WebKit:main Mar 2, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 2, 2023

philn deleted the eng/251428 branch March 2, 2023 18:45

Conversation

philn commented Jan 31, 2023 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

7878d72

Uh oh!

webkit-early-warning-system commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calvaris left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philn commented Feb 1, 2023

Uh oh!

calvaris commented Feb 2, 2023

Uh oh!

philn commented Feb 2, 2023

Uh oh!

Uh oh!

calvaris commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philn commented Feb 3, 2023

Uh oh!

calvaris commented Feb 7, 2023

Uh oh!

philn commented Feb 7, 2023

Uh oh!

calvaris commented Feb 8, 2023

Uh oh!

webkit-early-warning-system commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Mar 2, 2023

Uh oh!

webkit-early-warning-system commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calvaris left a comment

Choose a reason for hiding this comment

Uh oh!

webkit-commit-queue commented Mar 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

philn commented Jan 31, 2023 •

edited by webkit-early-warning-system

Loading

`7878d72`

webkit-early-warning-system commented Jan 31, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 31, 2023 •

edited

Loading

calvaris left a comment •

edited

Loading

calvaris commented Feb 3, 2023 •

edited

Loading

webkit-early-warning-system commented Mar 1, 2023 •

edited

Loading

webkit-early-warning-system commented Mar 2, 2023 •

edited

Loading