Commit f723a230 authored by Guido Urdaneta's avatar Guido Urdaneta Committed by Chromium LUCI CQ

[MediaStream] Make MediaStreamVideoSource::GetWeakPtr() pure virtual

This makes this method pure virtual and implements it in all derived
classes.
Prior to this CL, MediaStreamVideoSource had a weak pointer factory,
which was incorrect as derived classes define more data fields which
could  become invalid before the factory invalidated outstanding
weak pointers.

Bug: 1158227
Change-Id: I31e6e0f624e9ba81b74e234b72c58f9dc5dbd585
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2588928
Auto-Submit: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Reviewed-by: default avatarBill Budge <bbudge@chromium.org>
Reviewed-by: default avatarElad Alon <eladalon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837108}
parent 457d32b4
...@@ -444,6 +444,10 @@ class PepperMediaStreamVideoTrackHost::VideoSource final ...@@ -444,6 +444,10 @@ class PepperMediaStreamVideoTrackHost::VideoSource final
host_->frame_deliverer_ = nullptr; host_->frame_deliverer_ = nullptr;
} }
base::WeakPtr<MediaStreamVideoSource> GetWeakPtr() const final {
return weak_factory_.GetWeakPtr();
}
private: private:
base::Optional<media::VideoCaptureFormat> GetCurrentFormat() const override { base::Optional<media::VideoCaptureFormat> GetCurrentFormat() const override {
if (host_) { if (host_) {
...@@ -456,6 +460,7 @@ class PepperMediaStreamVideoTrackHost::VideoSource final ...@@ -456,6 +460,7 @@ class PepperMediaStreamVideoTrackHost::VideoSource final
} }
const base::WeakPtr<PepperMediaStreamVideoTrackHost> host_; const base::WeakPtr<PepperMediaStreamVideoTrackHost> host_;
base::WeakPtrFactory<MediaStreamVideoSource> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(VideoSource); DISALLOW_COPY_AND_ASSIGN(VideoSource);
}; };
......
...@@ -182,9 +182,7 @@ class BLINK_MODULES_EXPORT MediaStreamVideoSource ...@@ -182,9 +182,7 @@ class BLINK_MODULES_EXPORT MediaStreamVideoSource
return tracks_.size(); return tracks_.size();
} }
base::WeakPtr<MediaStreamVideoSource> GetWeakPtr() { virtual base::WeakPtr<MediaStreamVideoSource> GetWeakPtr() const = 0;
return weak_factory_.GetWeakPtr();
}
protected: protected:
// MediaStreamSource implementation. // MediaStreamSource implementation.
...@@ -365,9 +363,6 @@ class BLINK_MODULES_EXPORT MediaStreamVideoSource ...@@ -365,9 +363,6 @@ class BLINK_MODULES_EXPORT MediaStreamVideoSource
// callback to notify the caller that the request is canceled. // callback to notify the caller that the request is canceled.
base::OnceClosure remove_last_track_callback_; base::OnceClosure remove_last_track_callback_;
// NOTE: Weak pointers must be invalidated before all other member variables.
base::WeakPtrFactory<MediaStreamVideoSource> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(MediaStreamVideoSource); DISALLOW_COPY_AND_ASSIGN(MediaStreamVideoSource);
}; };
......
...@@ -165,6 +165,11 @@ void MediaStreamVideoCapturerSource::ChangeSourceImpl( ...@@ -165,6 +165,11 @@ void MediaStreamVideoCapturerSource::ChangeSourceImpl(
WTF::Unretained(this), capture_params_)); WTF::Unretained(this), capture_params_));
} }
base::WeakPtr<MediaStreamVideoSource>
MediaStreamVideoCapturerSource::GetWeakPtr() const {
return weak_factory_.GetWeakPtr();
}
void MediaStreamVideoCapturerSource::OnRunStateChanged( void MediaStreamVideoCapturerSource::OnRunStateChanged(
const media::VideoCaptureParams& new_capture_params, const media::VideoCaptureParams& new_capture_params,
bool is_running) { bool is_running) {
......
...@@ -81,6 +81,7 @@ class MODULES_EXPORT MediaStreamVideoCapturerSource ...@@ -81,6 +81,7 @@ class MODULES_EXPORT MediaStreamVideoCapturerSource
base::Optional<media::VideoCaptureParams> GetCurrentCaptureParams() base::Optional<media::VideoCaptureParams> GetCurrentCaptureParams()
const override; const override;
void ChangeSourceImpl(const MediaStreamDevice& new_device) override; void ChangeSourceImpl(const MediaStreamDevice& new_device) override;
base::WeakPtr<MediaStreamVideoSource> GetWeakPtr() const override;
// Method to bind as RunningCallback in VideoCapturerSource::StartCapture(). // Method to bind as RunningCallback in VideoCapturerSource::StartCapture().
void OnRunStateChanged(const media::VideoCaptureParams& new_capture_params, void OnRunStateChanged(const media::VideoCaptureParams& new_capture_params,
...@@ -108,6 +109,8 @@ class MODULES_EXPORT MediaStreamVideoCapturerSource ...@@ -108,6 +109,8 @@ class MODULES_EXPORT MediaStreamVideoCapturerSource
VideoCaptureDeliverFrameCB frame_callback_; VideoCaptureDeliverFrameCB frame_callback_;
DeviceCapturerFactoryCallback device_capturer_factory_callback_; DeviceCapturerFactoryCallback device_capturer_factory_callback_;
base::WeakPtrFactory<MediaStreamVideoSource> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(MediaStreamVideoCapturerSource); DISALLOW_COPY_AND_ASSIGN(MediaStreamVideoCapturerSource);
}; };
......
...@@ -142,8 +142,8 @@ void MediaStreamVideoSource::RemoveTrack(MediaStreamVideoTrack* video_track, ...@@ -142,8 +142,8 @@ void MediaStreamVideoSource::RemoveTrack(MediaStreamVideoTrack* video_track,
// sources created after that StopSource() call, but before the actual // sources created after that StopSource() call, but before the actual
// stop takes place. See https://crbug.com/778039. // stop takes place. See https://crbug.com/778039.
remove_last_track_callback_ = std::move(callback); remove_last_track_callback_ = std::move(callback);
StopForRestart(WTF::Bind(&MediaStreamVideoSource::DidStopSource, StopForRestart(
weak_factory_.GetWeakPtr())); WTF::Bind(&MediaStreamVideoSource::DidStopSource, GetWeakPtr()));
if (state_ == STOPPING_FOR_RESTART || state_ == STOPPED_FOR_RESTART) { if (state_ == STOPPING_FOR_RESTART || state_ == STOPPED_FOR_RESTART) {
// If the source supports restarting, it is necessary to call // If the source supports restarting, it is necessary to call
// FinalizeStopSource() to ensure the same behavior as StopSource(), // FinalizeStopSource() to ensure the same behavior as StopSource(),
...@@ -457,8 +457,8 @@ void MediaStreamVideoSource::StartFrameMonitoring() { ...@@ -457,8 +457,8 @@ void MediaStreamVideoSource::StartFrameMonitoring() {
GetTrackAdapter()->SetSourceFrameSize(current_format->frame_size); GetTrackAdapter()->SetSourceFrameSize(current_format->frame_size);
} }
GetTrackAdapter()->StartFrameMonitoring( GetTrackAdapter()->StartFrameMonitoring(
frame_rate, WTF::BindRepeating(&MediaStreamVideoSource::SetMutedState, frame_rate,
weak_factory_.GetWeakPtr())); WTF::BindRepeating(&MediaStreamVideoSource::SetMutedState, GetWeakPtr()));
} }
void MediaStreamVideoSource::SetReadyState( void MediaStreamVideoSource::SetReadyState(
......
...@@ -66,6 +66,11 @@ void MockMediaStreamVideoSource::OnHasConsumers(bool has_consumers) { ...@@ -66,6 +66,11 @@ void MockMediaStreamVideoSource::OnHasConsumers(bool has_consumers) {
is_suspended_ = !has_consumers; is_suspended_ = !has_consumers;
} }
base::WeakPtr<MediaStreamVideoSource> MockMediaStreamVideoSource::GetWeakPtr()
const {
return weak_factory_.GetWeakPtr();
}
void MockMediaStreamVideoSource::DoChangeSource( void MockMediaStreamVideoSource::DoChangeSource(
const MediaStreamDevice& new_device) { const MediaStreamDevice& new_device) {
ChangeSourceImpl(new_device); ChangeSourceImpl(new_device);
......
...@@ -71,6 +71,7 @@ class MockMediaStreamVideoSource : public blink::MediaStreamVideoSource { ...@@ -71,6 +71,7 @@ class MockMediaStreamVideoSource : public blink::MediaStreamVideoSource {
base::Optional<media::VideoCaptureParams> GetCurrentCaptureParams() base::Optional<media::VideoCaptureParams> GetCurrentCaptureParams()
const override; const override;
void OnHasConsumers(bool has_consumers) override; void OnHasConsumers(bool has_consumers) override;
base::WeakPtr<MediaStreamVideoSource> GetWeakPtr() const override;
protected: protected:
// Implements MediaStreamSource. // Implements MediaStreamSource.
...@@ -98,6 +99,8 @@ class MockMediaStreamVideoSource : public blink::MediaStreamVideoSource { ...@@ -98,6 +99,8 @@ class MockMediaStreamVideoSource : public blink::MediaStreamVideoSource {
blink::VideoCaptureDeliverFrameCB frame_callback_; blink::VideoCaptureDeliverFrameCB frame_callback_;
EncodedVideoFrameCB encoded_frame_callback_; EncodedVideoFrameCB encoded_frame_callback_;
base::WeakPtrFactory<MediaStreamVideoSource> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(MockMediaStreamVideoSource); DISALLOW_COPY_AND_ASSIGN(MockMediaStreamVideoSource);
}; };
......
...@@ -48,4 +48,9 @@ void PushableMediaStreamVideoSource::StopSourceImpl() { ...@@ -48,4 +48,9 @@ void PushableMediaStreamVideoSource::StopSourceImpl() {
running_ = false; running_ = false;
} }
base::WeakPtr<MediaStreamVideoSource>
PushableMediaStreamVideoSource::GetWeakPtr() const {
return weak_factory_.GetWeakPtr();
}
} // namespace blink } // namespace blink
...@@ -32,11 +32,14 @@ class MODULES_EXPORT PushableMediaStreamVideoSource ...@@ -32,11 +32,14 @@ class MODULES_EXPORT PushableMediaStreamVideoSource
void StartSourceImpl(VideoCaptureDeliverFrameCB frame_callback, void StartSourceImpl(VideoCaptureDeliverFrameCB frame_callback,
EncodedVideoFrameCB encoded_frame_callback) override; EncodedVideoFrameCB encoded_frame_callback) override;
void StopSourceImpl() override; void StopSourceImpl() override;
base::WeakPtr<MediaStreamVideoSource> GetWeakPtr() const override;
private: private:
bool running_ = false; bool running_ = false;
VideoCaptureDeliverFrameCB deliver_frame_cb_; VideoCaptureDeliverFrameCB deliver_frame_cb_;
THREAD_CHECKER(thread_checker_); THREAD_CHECKER(thread_checker_);
base::WeakPtrFactory<MediaStreamVideoSource> weak_factory_{this};
}; };
} // namespace blink } // namespace blink
......
...@@ -500,6 +500,11 @@ void MediaStreamRemoteVideoSource::RequestRefreshFrame() { ...@@ -500,6 +500,11 @@ void MediaStreamRemoteVideoSource::RequestRefreshFrame() {
} }
} }
base::WeakPtr<MediaStreamVideoSource> MediaStreamRemoteVideoSource::GetWeakPtr()
const {
return weak_factory_.GetWeakPtr();
}
void MediaStreamRemoteVideoSource::OnEncodedSinkEnabled() { void MediaStreamRemoteVideoSource::OnEncodedSinkEnabled() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
if (!observer_ || !observer_->track()) { if (!observer_ || !observer_->track()) {
......
...@@ -36,6 +36,7 @@ class MODULES_EXPORT MediaStreamRemoteVideoSource ...@@ -36,6 +36,7 @@ class MODULES_EXPORT MediaStreamRemoteVideoSource
// MediaStreamVideoSource overrides. // MediaStreamVideoSource overrides.
bool SupportsEncodedOutput() const override; bool SupportsEncodedOutput() const override;
void RequestRefreshFrame() override; void RequestRefreshFrame() override;
base::WeakPtr<MediaStreamVideoSource> GetWeakPtr() const override;
protected: protected:
// Implements MediaStreamVideoSource. // Implements MediaStreamVideoSource.
...@@ -60,6 +61,8 @@ class MODULES_EXPORT MediaStreamRemoteVideoSource ...@@ -60,6 +61,8 @@ class MODULES_EXPORT MediaStreamRemoteVideoSource
scoped_refptr<RemoteVideoSourceDelegate> delegate_; scoped_refptr<RemoteVideoSourceDelegate> delegate_;
std::unique_ptr<TrackObserver> observer_; std::unique_ptr<TrackObserver> observer_;
base::WeakPtrFactory<MediaStreamVideoSource> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(MediaStreamRemoteVideoSource); DISALLOW_COPY_AND_ASSIGN(MediaStreamRemoteVideoSource);
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment