New issue
Advanced search Search tips

Issue 618819 link

Starred by 2 users
Status: Fixed
Owner:
guidou@chromium.org
Closed: Jan 17
Cc:
guidou@chromium.org
m...@chromium.org
hta@chromium.org
haraken@chromium.org
niklase@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 921006


Sign in to add a comment

MediaStream{Descriptor,Source}: do not use ExtraData

Project Member Reported by mcasas@chromium.org, Jun 9 2016 
WebKit/Source/platform/mediastream/MediaStream{Descriptor,Source}
make use of an opaque OwnPtr<ExtraData> [1] to control content/ 
objects. This is a source of use-after-free errors and in 
general not a good programming technique.

Instead, as used elsewhere, the different content/ objects being
plugged in as ExtraData should be implementing interfaces 
defined in WebKit/Source/platform/mediastream (as e.g. [2])
and in those cases, they should probably use WebPrivatePtr<WebXXX>.

As a side note, the use of those opaque pointers was caused by
content/ class hierarchies being separated for audio and video,
but that problem was addressed about 2 years ago (see 3,4] and 
there are now e.g. content::MediaStreamSource [5] 
content::MediaStreamTrack [6] interfaces acting as base classes 
for both Audio and Video.


[1] https://cs.chromium.org/search/?q=mediastream+OwnPtr%3CExtraData%3E+m_extraData&sq=package:chromium&type=cs
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mediastream/MediaStreamCenter.h?dr=CSs&sq=package:chromium&rcl=1465485429&l=76
[3] (Some old Design Doc where those abstractions stem from)
https://drive.google.com/open?id=1H13NEjSjYZg63v24lonx8MPomxsGBCp2C7e5bRArZoQ
[4] https://chromium.googlesource.com/chromium/src/+/master/docs/piranha_plant.md
[5] https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_source.h?q=MediaStreamSource&sq=package:chromium&l=23&dr=C
[6] https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_track.h?q=content/+MediaStreamTrack&sq=package:chromium&l=20&dr=C

Comment 1 by mcasas@chromium.org, Jun 20 2016

Status: Available (was: Untriaged)
Ping. hta@, guidou@ WDYT?

Comment 2 by hta@chromium.org, Jun 21 2016 

Thanks for digging up all this information!

MediaDescriptor's ExtraData has already been renamed to TrackData and given an API (in order to support getSettings).

The same thing will happen to MediaSource; we need a defined API there too. So the opaqueness is going away, which helps readability.

OwnPtr is gone as of this weekend, I read the announcement as it being replaced with std::unique_ptr. This takes care of the ownership issue, I think.

I don't see it as an urgent matter; pri 3 seems about right.

Comment 3 by guidou@chromium.org, Feb 27 2017

Owner: guidou@chromium.org

Comment 4 by mcasas@chromium.org, Feb 27 2017

Status: Assigned (was: Available)
If Owner != 'undefined' then status ==> 'Asssigned'

:-)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94e83cf5a5c9353ab54ba31b82de6c6320917d49

commit 94e83cf5a5c9353ab54ba31b82de6c6320917d49
Author: Guido Urdaneta <guidou@chromium.org>
Date: Tue Aug 29 07:25:48 2017

Remove content::MediaStream

The only functionality it had was handling a list of observers.
This functionality has been moved to Blink.

Bug:  618819 
Change-Id: Ia00f627bf6525099ac5797c08293942b9ab054f6
Reviewed-on: https://chromium-review.googlesource.com/632579
Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498039}
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/BUILD.gn
[delete] https://crrev.com/7f01af5117393ace7a47799516ee631283c81ccf/content/renderer/media/media_stream.cc
[delete] https://crrev.com/7f01af5117393ace7a47799516ee631283c81ccf/content/renderer/media/media_stream.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/media_stream_center.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/media_stream_center.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/media_stream_renderer_factory_impl.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/mock_media_stream_registry.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/pepper_to_video_track_adapter.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/pepper_to_video_track_adapter_unittest.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/user_media_client_impl.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/user_media_client_impl_unittest.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/video_track_to_pepper_adapter.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/video_track_to_pepper_adapter_unittest.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/webmediaplayer_ms.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/webmediaplayer_ms.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/webrtc/peer_connection_dependency_factory.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/webrtc/webrtc_media_stream_adapter.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/webrtc/webrtc_media_stream_adapter.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/webrtc/webrtc_media_stream_adapter_map_unittest.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/renderer/media/webrtc/webrtc_media_stream_adapter_unittest.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/shell/test_runner/mock_web_media_stream_center.cc
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/content/shell/test_runner/mock_web_media_stream_center.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/Source/modules/mediastream/MediaStream.cpp
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/Source/platform/exported/WebMediaStream.cpp
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/Source/platform/mediastream/MediaStreamCenter.cpp
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/Source/platform/mediastream/MediaStreamCenter.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.cpp
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/public/platform/WebMediaStream.h
[modify] https://crrev.com/94e83cf5a5c9353ab54ba31b82de6c6320917d49/third_party/WebKit/public/platform/WebMediaStreamCenter.h

Comment 6 by guidou@chromium.org, Jan 11

Blockedon: 921006
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 17 (6 days ago) 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c409af4de13ed229b7f462c8545803b5fafe395

commit 3c409af4de13ed229b7f462c8545803b5fafe395
Author: Guido Urdaneta <guidou@chromium.org>
Date: Thu Jan 17 18:18:14 2019

Remove MediaStreamSource::ExtraData

This CL moves the WebMediaStreamSource::ExtraData functionality to
PlatformMediaStreamSource.

Drive-by: minor refactoring in UserMediaProcessor to return unique_ptr
instead of raw pointers in source-creation functions.

Bug: 764293,  618819 , 921006
Change-Id: I9e42201bff07f96996733f3d952b393f357da523
TBR: haraken@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1408248
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623754}
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/public/renderer/media_stream_utils.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_audio_source.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_audio_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_center.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_video_capturer_source_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_video_renderer_sink_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_video_source.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_video_source_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/media_stream_video_track_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/mock_media_stream_registry.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/processed_local_audio_source_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/remote_media_stream_track_adapter.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/user_media_client_impl_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/user_media_processor.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/stream/user_media_processor.h
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/fake_rtc_rtp_transceiver.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/rtc_rtp_sender_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/rtc_rtp_transceiver_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/transceiver_state_surfacer_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/webrtc_media_stream_track_adapter_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media/webrtc/webrtc_set_description_observer_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media_capture_from_element/canvas_capture_handler.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media_capture_from_element/canvas_capture_handler_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media_capture_from_element/html_audio_element_capturer_source_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media_recorder/audio_track_recorder_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/media_recorder/video_track_recorder_unittest.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/pepper/pepper_media_stream_video_track_host.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/third_party/blink/public/platform/modules/mediastream/platform_media_stream_source.h
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/third_party/blink/public/platform/web_media_stream_source.h
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/third_party/blink/renderer/platform/exported/platform_media_stream_source.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/third_party/blink/renderer/platform/exported/web_media_stream_source.cc
[modify] https://crrev.com/3c409af4de13ed229b7f462c8545803b5fafe395/third_party/blink/renderer/platform/mediastream/media_stream_source.h
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 17 (6 days ago) 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2aca21845fcba5cdb8dd19605083b2476189b58e

commit 2aca21845fcba5cdb8dd19605083b2476189b58e
Author: Guido Urdaneta <guidou@chromium.org>
Date: Thu Jan 17 21:23:58 2019

Remove MediaStreamSource::TrackData.

Bug:  618819 , 921006
Change-Id: I47163857152803d6b52a680cdeac28f9eb6bf32d
Reviewed-on: https://chromium-review.googlesource.com/c/1406649
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623849}
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/image_capture/image_capture_frame_grabber.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/stream/media_stream_audio_source.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/stream/media_stream_audio_track.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/stream/media_stream_center.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/stream/media_stream_video_track.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/stream/mock_media_stream_registry.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/stream/remote_media_stream_track_adapter.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media/webrtc_local_audio_source_provider_unittest.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media_capture_from_element/canvas_capture_handler.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media_recorder/video_track_recorder.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/content/renderer/media_recorder/video_track_recorder_unittest.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/third_party/blink/public/platform/modules/mediastream/platform_media_stream_track.h
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/third_party/blink/public/platform/web_media_stream_track.h
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/third_party/blink/renderer/platform/exported/platform_media_stream_track.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/third_party/blink/renderer/platform/exported/web_media_stream_track.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/third_party/blink/renderer/platform/mediastream/media_stream_component.cc
[modify] https://crrev.com/2aca21845fcba5cdb8dd19605083b2476189b58e/third_party/blink/renderer/platform/mediastream/media_stream_component.h

Comment 9 by guidou@chromium.org, Jan 17 (6 days ago)

Status: Fixed (was: Assigned)
►

Sign in to add a comment