Issue 13813016: Remove reference counting from media::Demuxer and friends.

Issue 13813016: Remove reference counting from media::Demuxer and friends. (Closed)

Created:
7 years, 8 months ago by scherkus (not reviewing)

Modified:
7 years, 8 months ago

Reviewers:
acolwell GONE FROM CHROMIUM

CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org

Base URL:
http://git.chromium.org/chromium/src.git@vd_scoped

Visibility:
Public.

More Reviews

Description

Remove reference counting from media::Demuxer and friends. In addition: * Pipeline no longer owns the demuxer; clients are required to own it and keep it alive until Pipeline::Stop() completes. BUG=173313 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195437

Patch Set 1 : #

Total comments: 29

Patch Set 2 : demuxer only #

Total comments: 4

Patch Set 3 : fixes #

Patch Set 4 : rebase #

Patch Set 5 : fix tools #

Total comments: 3

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+220 lines, -231 lines)			Patch
M	media/base/demuxer.h	View	1	3 chunks	+2 lines, -6 lines	0 comments	Download
M	media/base/filter_collection.h	View	1	3 chunks	+3 lines, -4 lines	0 comments	Download
M	media/base/filter_collection.cc	View	1	1 chunk	+3 lines, -4 lines	0 comments	Download
M	media/base/mock_filters.h	View	1	2 chunks	+1 line, -3 lines	0 comments	Download
M	media/base/pipeline.h	View	1	2 chunks	+8 lines, -7 lines	0 comments	Download
M	media/base/pipeline.cc	View	1 2 3	5 chunks	+7 lines, -9 lines	0 comments	Download
M	media/base/pipeline_unittest.cc	View	1	2 chunks	+4 lines, -4 lines	0 comments	Download
M	media/filters/chunk_demuxer.h	View	1	2 chunks	+1 line, -3 lines	0 comments	Download
M	media/filters/chunk_demuxer_unittest.cc	View	1	12 chunks	+20 lines, -24 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer.h	View	1	4 chunks	+3 lines, -3 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer.cc	View	1 2 3	12 chunks	+14 lines, -10 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer_unittest.cc	View	1	4 chunks	+6 lines, -10 lines	0 comments	Download
M	media/filters/pipeline_integration_test.cc	View	1	8 chunks	+17 lines, -11 lines	0 comments	Download
M	media/filters/pipeline_integration_test_base.h	View	1	4 chunks	+3 lines, -2 lines	0 comments	Download
M	media/filters/pipeline_integration_test_base.cc	View	1	1 chunk	+7 lines, -7 lines	0 comments	Download
M	media/tools/demuxer_bench/demuxer_bench.cc	View	1 2 3 4	3 chunks	+5 lines, -6 lines	0 comments	Download
M	media/tools/player_x11/player_x11.cc	View	1	3 chunks	+5 lines, -6 lines	0 comments	Download
M	media/tools/seek_tester/seek_tester.cc	View	1 2 3 4	1 chunk	+2 lines, -3 lines	0 comments	Download
M	webkit/media/webmediaplayer_impl.h	View	1 2	4 chunks	+6 lines, -6 lines	2 comments	Download
M	webkit/media/webmediaplayer_impl.cc	View	1 2	7 chunks	+95 lines, -91 lines	1 comment	Download
M	webkit/media/webmediasourceclient_impl.h	View	1	3 chunks	+2 lines, -5 lines	0 comments	Download
M	webkit/media/webmediasourceclient_impl.cc	View	1	1 chunk	+1 line, -2 lines	0 comments	Download
M	webkit/media/websourcebuffer_impl.h	View	1	3 chunks	+4 lines, -4 lines	0 comments	Download
M	webkit/media/websourcebuffer_impl.cc	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 12 (0 generated)

Expand Messages | Collapse Messages

scherkus (not reviewing)

got some q's for ya https://codereview.chromium.org/13813016/diff/5001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/13813016/diff/5001/media/base/pipeline.cc#newcode662 media/base/pipeline.cc:662: demuxer_.reset(); I'm still skeptical ...

7 years, 8 months ago (2013-04-17 17:21:59 UTC) #1

acolwell GONE FROM CHROMIUM

Here are some initial comments. I think we should split the Demuxer & DemuxerStream changes ...

7 years, 8 months ago (2013-04-17 20:24:53 UTC) #2

scherkus (not reviewing)

acolwell: PTAL (subject changed) https://codereview.chromium.org/13813016/diff/5001/media/base/filter_collection.h File media/base/filter_collection.h (right): https://codereview.chromium.org/13813016/diff/5001/media/base/filter_collection.h#newcode28 media/base/filter_collection.h:28: scoped_ptr<Demuxer> GetDemuxer(); On 2013/04/17 20:24:53, ...

7 years, 8 months ago (2013-04-19 01:07:22 UTC) #3

acolwell GONE FROM CHROMIUM

Looks pretty good to me. Just a few minor nits and waiting to see why ...

7 years, 8 months ago (2013-04-19 18:17:59 UTC) #4

acolwell GONE FROM CHROMIUM

removed 'and friends' from CL description since refcounting was only removed from Demuxer.

7 years, 8 months ago (2013-04-19 18:19:30 UTC) #5

scherkus (not reviewing)

added friends back! PTAL https://codereview.chromium.org/13813016/diff/19001/media/base/pipeline.cc File media/base/pipeline.cc (left): https://codereview.chromium.org/13813016/diff/19001/media/base/pipeline.cc#oldcode660 media/base/pipeline.cc:660: demuxer_ = NULL; On 2013/04/19 ...

7 years, 8 months ago (2013-04-19 23:18:45 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scherkus@chromium.org/13813016/13030

7 years, 8 months ago (2013-04-19 23:55:57 UTC) #8

commit-bot: I haz the power

Failed to apply patch for webkit/media/webmediaplayer_impl.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 8 months ago (2013-04-19 23:56:03 UTC) #9

Failed to apply patch for webkit/media/webmediaplayer_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file webkit/media/webmediaplayer_impl.cc
  Hunk #7 FAILED at 1332.
  1 out of 7 hunks FAILED -- saving rejects to file
webkit/media/webmediaplayer_impl.cc.rej

Patch:       webkit/media/webmediaplayer_impl.cc
Index: webkit/media/webmediaplayer_impl.cc
diff --git a/webkit/media/webmediaplayer_impl.cc
b/webkit/media/webmediaplayer_impl.cc
index
ce2cbc7c78b80aec988fe38ac667cdf0204f7347..8c339ca88d7882f20d0d524b127ee7f18001b464
100644
--- a/webkit/media/webmediaplayer_impl.cc
+++ b/webkit/media/webmediaplayer_impl.cc
@@ -153,6 +153,7 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
       is_local_source_(false),
       supports_save_(true),
       starting_(false),
+      chunk_demuxer_(NULL),
       pending_repaint_(false),
       video_frame_provider_client_(NULL) {
   media_log_->AddEvent(
@@ -266,18 +267,11 @@ void WebMediaPlayerImpl::load(const WebKit::WebURL& url,
CORSMode cors_mode) {
 void WebMediaPlayerImpl::load(const WebKit::WebURL& url,
                               WebKit::WebMediaSource* media_source,
                               CORSMode cors_mode) {
-  scoped_ptr<WebKit::WebMediaSource> ms(media_source);
   LoadSetup(url);
 
   // Media source pipelines can start immediately.
-  chunk_demuxer_ = new media::ChunkDemuxer(
-      BIND_TO_RENDER_LOOP_1(&WebMediaPlayerImpl::OnDemuxerOpened,
-                            base::Passed(&ms)),
-      BIND_TO_RENDER_LOOP_2(&WebMediaPlayerImpl::OnNeedKey, "", ""),
-      base::Bind(&LogMediaSourceError, media_log_));
-
   supports_save_ = false;
-  StartPipeline();
+  StartPipeline(media_source);
 }
 
 void WebMediaPlayerImpl::LoadSetup(const WebKit::WebURL& url) {
@@ -1122,7 +1116,7 @@ void WebMediaPlayerImpl::DataSourceInitialized(const GURL&
gurl, bool success) {
     return;
   }
 
-  StartPipeline();
+  StartPipeline(NULL);
 }
 
 void WebMediaPlayerImpl::NotifyDownloading(bool is_downloading) {
@@ -1136,10 +1130,96 @@ void WebMediaPlayerImpl::NotifyDownloading(bool
is_downloading) {
           "is_downloading_data", is_downloading));
 }
 
-void WebMediaPlayerImpl::StartPipeline() {
+void WebMediaPlayerImpl::StartPipeline(WebKit::WebMediaSource* media_source) {
+  const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
+
+
+  // Figure out which demuxer to use.
+  if (!media_source) {
+    DCHECK(!chunk_demuxer_);
+    DCHECK(data_source_);
+
+    demuxer_.reset(new media::FFmpegDemuxer(
+        media_thread_.message_loop_proxy(), data_source_,
+        BIND_TO_RENDER_LOOP_2(&WebMediaPlayerImpl::OnNeedKey, "", "")));
+  } else {
+    DCHECK(!chunk_demuxer_);
+    DCHECK(!data_source_);
+
+    scoped_ptr<WebKit::WebMediaSource> ms(media_source);
+    chunk_demuxer_ = new media::ChunkDemuxer(
+        BIND_TO_RENDER_LOOP_1(&WebMediaPlayerImpl::OnDemuxerOpened,
+                              base::Passed(&ms)),
+        BIND_TO_RENDER_LOOP_2(&WebMediaPlayerImpl::OnNeedKey, "", ""),
+        base::Bind(&LogMediaSourceError, media_log_));
+    demuxer_.reset(chunk_demuxer_);
+
+    // Disable GpuVideoDecoder creation until it supports codec config changes.
+    // TODO(acolwell): Remove this once http://crbug.com/151045 is fixed.
+    gpu_factories_ = NULL;
+  }
+
+  scoped_ptr<media::FilterCollection> filter_collection(
+      new media::FilterCollection());
+  filter_collection->SetDemuxer(demuxer_.get());
+
+  // Figure out if EME is enabled.
+  media::SetDecryptorReadyCB set_decryptor_ready_cb;
+  if (decryptor_) {
+    set_decryptor_ready_cb = base::Bind(&ProxyDecryptor::SetDecryptorReadyCB,
+                                        base::Unretained(decryptor_.get()));
+  }
+
+  // Create our audio decoders and renderer.
+  ScopedVector<media::AudioDecoder> audio_decoders;
+  audio_decoders.push_back(new media::FFmpegAudioDecoder(
+      media_thread_.message_loop_proxy()));
+  if (cmd_line->HasSwitch(switches::kEnableOpusPlayback)) {
+    audio_decoders.push_back(new media::OpusAudioDecoder(
+        media_thread_.message_loop_proxy()));
+  }
+
+  scoped_ptr<media::AudioRenderer> audio_renderer(
+      new media::AudioRendererImpl(media_thread_.message_loop_proxy(),
+                                   audio_source_provider_,
+                                   audio_decoders.Pass(),
+                                   set_decryptor_ready_cb));
+  filter_collection->SetAudioRenderer(audio_renderer.Pass());
+
+  // Create our video decoders and renderer.
+  ScopedVector<media::VideoDecoder> video_decoders;
+
+  if (gpu_factories_) {
+    video_decoders.push_back(new media::GpuVideoDecoder(
+        media_thread_.message_loop_proxy(), gpu_factories_));
+  }
+
+  video_decoders.push_back(new media::FFmpegVideoDecoder(
+      media_thread_.message_loop_proxy()));
+
+  // TODO(phajdan.jr): Remove ifdefs when libvpx with vp9 support is released
+  // (http://crbug.com/174287) .
+#if !defined(MEDIA_DISABLE_LIBVPX)
+  if (cmd_line->HasSwitch(switches::kEnableVp9Playback)) {
+    video_decoders.push_back(new media::VpxVideoDecoder(
+        media_thread_.message_loop_proxy()));
+  }
+#endif  // !defined(MEDIA_DISABLE_LIBVPX)
+
+  scoped_ptr<media::VideoRenderer> video_renderer(
+      new media::VideoRendererBase(
+          media_thread_.message_loop_proxy(),
+          video_decoders.Pass(),
+          set_decryptor_ready_cb,
+          base::Bind(&WebMediaPlayerImpl::FrameReady, base::Unretained(this)),
+          BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::SetOpaque),
+          true));
+  filter_collection->SetVideoRenderer(video_renderer.Pass());
+
+  // ... and we're ready to go!
   starting_ = true;
   pipeline_->Start(
-      BuildFilterCollection(),
+      filter_collection.Pass(),
       BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineEnded),
       BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineError),
       BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineSeek),
@@ -1175,8 +1255,10 @@ void WebMediaPlayerImpl::Destroy() {
   // Abort any pending IO so stopping the pipeline doesn't get blocked.
   if (data_source_)
     data_source_->Abort();
-  if (chunk_demuxer_)
+  if (chunk_demuxer_) {
     chunk_demuxer_->Shutdown();
+    chunk_demuxer_ = NULL;
+  }
 
   if (gpu_factories_) {
     gpu_factories_->Abort();
@@ -1200,7 +1282,7 @@ void WebMediaPlayerImpl::Destroy() {
 
   // Release any final references now that everything has stopped.
   data_source_ = NULL;
-  chunk_demuxer_ = NULL;
+  demuxer_.reset();
 }
 
 WebKit::WebMediaPlayerClient* WebMediaPlayerImpl::GetClient() {
@@ -1250,82 +1332,4 @@ void WebMediaPlayerImpl::FrameReady(
       &WebMediaPlayerImpl::Repaint, AsWeakPtr()));
 }
 
-scoped_ptr<media::FilterCollection>
-WebMediaPlayerImpl::BuildFilterCollection() {
-  const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
-
-  scoped_ptr<media::FilterCollection> filter_collection(
-      new media::FilterCollection());
-
-  // Figure out which demuxer to use.
-  if (data_source_) {
-    DCHECK(!chunk_demuxer_);
-    filter_collection->SetDemuxer(new media::FFmpegDemuxer(
-        media_thread_.message_loop_proxy(), data_source_,
-        BIND_TO_RENDER_LOOP_2(&WebMediaPlayerImpl::OnNeedKey, "", "")));
-  } else {
-    DCHECK(chunk_demuxer_);
-    filter_collection->SetDemuxer(chunk_demuxer_);
-
-    // Disable GpuVideoDecoder creation until it supports codec config changes.
-    // TODO(acolwell): Remove this once http://crbug.com/151045 is fixed.
-    gpu_factories_ = NULL;
-  }
-
-  // Figure out if EME is enabled.
-  media::SetDecryptorReadyCB set_decryptor_ready_cb;
-  if (decryptor_) {
-    set_decryptor_ready_cb = base::Bind(&ProxyDecryptor::SetDecryptorReadyCB,
-                                        base::Unretained(decryptor_.get()));
-  }
-
-  // Create our audio decoders and renderer.
-  ScopedVector<media::AudioDecoder> audio_decoders;
-  audio_decoders.push_back(new media::FFmpegAudioDecoder(
-      media_thread_.message_loop_proxy()));
-  if (cmd_line->HasSwitch(switches::kEnableOpusPlayback)) {
-    audio_decoders.push_back(new media::OpusAudioDecoder(
-        media_thread_.message_loop_proxy()));
-  }
-
-  scoped_ptr<media::AudioRenderer> audio_renderer(
-      new media::AudioRendererImpl(media_thread_.message_loop_proxy(),
-                                   audio_source_provider_,
-                                   audio_decoders.Pass(),
-                                   set_decryptor_ready_cb));
-  filter_collection->SetAudioRenderer(audio_renderer.Pass());
-
-  // Create our video decoders and renderer.
-  ScopedVector<media::VideoDecoder> video_decoders;
-
-  if (gpu_factories_) {
-    video_decoders.push_back(new media::GpuVideoDecoder(
-        media_thread_.message_loop_proxy(), gpu_factories_));
-  }
-
-  video_decoders.push_back(new media::FFmpegVideoDecoder(
-      media_thread_.message_loop_proxy()));
-
-  // TODO(phajdan.jr): Remove ifdefs when libvpx with vp9 support is released
-  // (http://crbug.com/174287) .
-#if !defined(MEDIA_DISABLE_LIBVPX)
-  if (cmd_line->HasSwitch(switches::kEnableVp9Playback)) {
-    video_decoders.push_back(new media::VpxVideoDecoder(
-        media_thread_.message_loop_proxy()));
-  }
-#endif  // !defined(MEDIA_DISABLE_LIBVPX)
-
-  scoped_ptr<media::VideoRenderer> video_renderer(
-      new media::VideoRendererBase(
-          media_thread_.message_loop_proxy(),
-          video_decoders.Pass(),
-          set_decryptor_ready_cb,
-          base::Bind(&WebMediaPlayerImpl::FrameReady, base::Unretained(this)),
-          BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::SetOpaque),
-          true));
-  filter_collection->SetVideoRenderer(video_renderer.Pass());
-
-  return filter_collection.Pass();
-}
-
 }  // namespace webkit_media

xhwang

drive-by nit https://codereview.chromium.org/13813016/diff/13030/webkit/media/webmediaplayer_impl.h File webkit/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/13813016/diff/13030/webkit/media/webmediaplayer_impl.h#newcode350 webkit/media/webmediaplayer_impl.h:350: // |chunk_demuxer_| is used for Media Source ...

7 years, 8 months ago (2013-04-20 00:09:54 UTC) #10

scherkus (not reviewing)

https://codereview.chromium.org/13813016/diff/13030/webkit/media/webmediaplayer_impl.h File webkit/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/13813016/diff/13030/webkit/media/webmediaplayer_impl.h#newcode350 webkit/media/webmediaplayer_impl.h:350: // |chunk_demuxer_| is used for Media Source resource loads. ...

7 years, 8 months ago (2013-04-22 04:02:07 UTC) #11

scherkus (not reviewing)

7 years, 8 months ago (2013-04-22 04:03:44 UTC) #12

Message was sent while issue was closed.

Committed patchset #5 manually as r195437 (presubmit successful).

Expand Messages | Collapse Messages