Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WebKit MediaRecorderPrivate should not be refcounted #18887

Merged
merged 1 commit into from
Oct 11, 2023
Merged

WebKit MediaRecorderPrivate should not be refcounted #18887

merged 1 commit into from
Oct 11, 2023

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Oct 10, 2023 •
edited by webkit-early-warning-system
Loading

5ac998d

WebKit MediaRecorderPrivate should not be refcounted
https://bugs.webkit.org/show_bug.cgi?id=262945
rdar://problem/116726041

Reviewed by Alex Christensen and Chris Dumez.

Given MediaRecorderPrivate is stored as a std::unique_ptr, it is best to not ref/deref.
Instead MediaRecorderPrivate stores a ref counted object that is observing GPUProcess connection closure.

* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:
(WebKit::MediaRecorderPrivate::MediaRecorderPrivate):
(WebKit::MediaRecorderPrivate::startRecording):
(WebKit::MediaRecorderPrivate::gpuProcessConnectionDidClose):
* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.h:

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

feae42e

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

@youennf youennf requested a review from cdumez as a code owner October 10, 2023 08:41
@youennf youennf self-assigned this Oct 10, 2023
@youennf youennf added the WebRTC For bugs in WebRTC label Oct 10, 2023
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Oct 10, 2023 •
edited
Loading

EWS run on current version of this PR (hash feae42e)

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

cdumez
cdumez approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess @JonWBedard hasn't landed the makeUniqueWithoutRefCountedCheck() yet since I don't see it fixed in this patch?

Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.h Show resolved Hide resolved
Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.h Show resolved Hide resolved
Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.h Show resolved Hide resolved
@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label Oct 11, 2023
@youennf
WebKit MediaRecorderPrivate should not be refcounted
5ac998d 
https://bugs.webkit.org/show_bug.cgi?id=262945
rdar://problem/116726041

Reviewed by Alex Christensen and Chris Dumez.

Given MediaRecorderPrivate is stored as a std::unique_ptr, it is best to not ref/deref.
Instead MediaRecorderPrivate stores a ref counted object that is observing GPUProcess connection closure.

* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:
(WebKit::MediaRecorderPrivate::MediaRecorderPrivate):
(WebKit::MediaRecorderPrivate::startRecording):
(WebKit::MediaRecorderPrivate::gpuProcessConnectionDidClose):
* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.h:

Canonical link: https://commits.webkit.org/269195@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/WebKit-MediaRecorderPrivate-should-not-be-refcounted branch from feae42e to 5ac998d Compare October 11, 2023 09:57
@webkit-commit-queue
Copy link
Collaborator

Committed 269195@main (5ac998d): https://commits.webkit.org/269195@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 11, 2023
@webkit-commit-queue webkit-commit-queue merged commit 5ac998d into WebKit:main Oct 11, 2023
@youennf
Copy link
Contributor Author

youennf commented Oct 11, 2023

Hum, I forgot to include the changes that were asked, will do in a follow-up

@youennf youennf mentioned this pull request Oct 11, 2023
Merged
webkit-commit-queue pushed a commit to youennf/WebKit that referenced this pull request Oct 11, 2023
@youennf
Address comments that were missing in https://commits.webkit.org/2691…
87c90b9 
…95@main

https://bugs.webkit.org/show_bug.cgi?id=263011
rdar://116806970

Reviewed by Alex Christensen and Chris Dumez.

I did not push all changes related to the review of WebKit#18887 in https://commits.webkit.org/269195@main.
Adding them here.

* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:
(WebKit::MediaRecorderPrivate::MediaRecorderPrivate):
* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.h:

Canonical link: https://commits.webkit.org/269207@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez approved these changes

@achristensen07 achristensen07 achristensen07 approved these changes

Assignees

@youennf youennf

Labels
WebRTC For bugs in WebRTC
Projects
None yet
Milestone
No milestone
5 participants
@youennf @webkit-early-warning-system @webkit-commit-queue @cdumez @achristensen07