Remove reference counting from media::DemuxerStream and friends.

Remove reference counting from media::DemuxerStream and friends.

BUG=173313
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196365

[scherkus (not reviewing), 2013-04-22 16:38:47]

here's another!

[acolwell GONE FROM CHROMIUM, 2013-04-23 03:20:42]

Looks good. Just a few comments.

https://codereview.chromium.org/14217008/diff/5052/media/filters/audio_decode...
File media/filters/audio_decoder_selector.cc (left):

https://codereview.chromium.org/14217008/diff/5052/media/filters/audio_decode...
media/filters/audio_decoder_selector.cc:27:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
Why isn't this needed anymore?

https://codereview.chromium.org/14217008/diff/5052/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/14217008/diff/5052/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:616: audio_.reset();  // XXX: IS THIS OK?
Unfortunately no. The decoder still has a reference. This needs to outlive the
decoder. Need to add a audio_disabled_ boolean to track this state. The existing
code may actually be quite broken. Basically we don't want buffers to get stored
in the stream and we want the stream to always return EOS for reads.

Do we really encounter this codepath a lot? We may want to rethink this
functionality in general... in another CL. :)

https://codereview.chromium.org/14217008/diff/5052/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer_unittest.cc (left):

https://codereview.chromium.org/14217008/diff/5052/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer_unittest.cc:483: TEST_F(FFmpegDemuxerTest,
StreamReadAfterStopAndDemuxerDestruction) {
I feel like this test is still useful w/o destruction. Is it possible for
Read()s to occur after Stop() is called on the demuxer?

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
File media/filters/video_frame_stream.cc (left):

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
media/filters/video_frame_stream.cc:128: stream_ = NULL;
nit: Any harm in keeping this?

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
media/filters/video_frame_stream.cc:239: stream_ = NULL;
nit:ditto

[xhwang, 2013-04-23 16:41:37]

drive-by comment

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:193: scoped_ptr<VideoFrameStream>
video_frame_stream_;
These members used to be scoped_refptr because we have to use scopted_refptr for
scoped_refptrs. Now they don't have to be scopted_ptrs, we can just use a member
variable. But that can be another CL as it applies to both demuxer_stream and
decoders.

[scherkus (not reviewing), 2013-04-23 18:43:40]

PTAL

https://codereview.chromium.org/14217008/diff/5052/media/filters/audio_decode...
File media/filters/audio_decoder_selector.cc (left):

https://codereview.chromium.org/14217008/diff/5052/media/filters/audio_decode...
media/filters/audio_decoder_selector.cc:27:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
On 2013/04/23 03:20:42, acolwell wrote:
> Why isn't this needed anymore?

I had a hunch that this macro hasn't been working for quite some time and wanted
to test it out.

Turns out it's been useless for ~2 years (!)

https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...

https://codereview.chromium.org/14217008/diff/5052/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/14217008/diff/5052/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:616: audio_.reset();  // XXX: IS THIS OK?
On 2013/04/23 03:20:42, acolwell wrote:
> Unfortunately no. The decoder still has a reference. This needs to outlive the
> decoder. Need to add a audio_disabled_ boolean to track this state. The
existing
> code may actually be quite broken. Basically we don't want buffers to get
stored
> in the stream and we want the stream to always return EOS for reads.
> 
> Do we really encounter this codepath a lot? We may want to rethink this
> functionality in general... in another CL. :)

I took a stab at fixing this + added a test, but I don't know enough about
ChunkDemuxer to be confident it won't break in some edge case :|

AFAIK we don't have any UMA stats for how often we hit this scenario.

Thinking out loud... it'd be nice if demuxers didn't have to worry about this.
The only reason why its here is so that demuxers won't keep demuxed audio data
around if we know audio has been disabled.

I wonder if with all the recent audio clean up
AudioRendererImpl::OnRenderError() ever gets called. Heck, we might end up
transparently falling back to a fake audio source!

(this code path used to get hit if, say, running on a Windows VM with sound
cards disabled -- see http://crbug.com/106735 for an example)

Filed http://crbug.com/234708 and annotated Demuxer::OARD() and AR::Initialize()

https://codereview.chromium.org/14217008/diff/5052/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer_unittest.cc (left):

https://codereview.chromium.org/14217008/diff/5052/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer_unittest.cc:483: TEST_F(FFmpegDemuxerTest,
StreamReadAfterStopAndDemuxerDestruction) {
On 2013/04/23 03:20:42, acolwell wrote:
> I feel like this test is still useful w/o destruction. Is it possible for
> Read()s to occur after Stop() is called on the demuxer?

Yes and that is covered by the test above (FFmpegDemuxerTest.Stop)

This test covered the case where we do a Read() but due to refcounting the
stream is kept alive but the demuxer was destroyed.

Although looking at the test again it seems silly that we're testing that the
callback is deleted ... it seems we should only care about running the callback
(whether the callback is deleted is an implementation detail)

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
File media/filters/video_frame_stream.cc (left):

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
media/filters/video_frame_stream.cc:128: stream_ = NULL;
On 2013/04/23 03:20:42, acolwell wrote:
> nit: Any harm in keeping this?

Done.

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
media/filters/video_frame_stream.cc:239: stream_ = NULL;
On 2013/04/23 03:20:42, acolwell wrote:
> nit:ditto

Done.

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/14217008/diff/5052/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:193: scoped_ptr<VideoFrameStream>
video_frame_stream_;
On 2013/04/23 16:41:37, xhwang wrote:
> These members used to be scoped_refptr because we have to use scopted_refptr
for
> scoped_refptrs. Now they don't have to be scopted_ptrs, we can just use a
member
> variable. But that can be another CL as it applies to both demuxer_stream and
> decoders.

Yeah acolwell@ and I discussed this in the previous CL:
https://codereview.chromium.org/13813016/diff/5001/media/filters/decrypting_a...

It's a lot of added noise to the CL so deferring for now.

[acolwell GONE FROM CHROMIUM, 2013-04-24 06:09:57]

lgtm

https://codereview.chromium.org/14217008/diff/16002/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/14217008/diff/16002/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:617: disabled_audio_ = audio_.Pass();
nit: DCHECK(!disabled_audio_) just so we can catch any case where this
accidentally gets called more than once?

https://codereview.chromium.org/14217008/diff/16002/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:627: return audio_.get() ? audio_.get() :
disabled_audio_.get();
Is this change actually needed? Doesn't this change the existing behavior when
OnAudioRendererDisabled() is called.

[scherkus (not reviewing), 2013-04-25 00:12:53]

https://codereview.chromium.org/14217008/diff/16002/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/14217008/diff/16002/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:617: disabled_audio_ = audio_.Pass();
On 2013/04/24 06:09:57, acolwell wrote:
> nit: DCHECK(!disabled_audio_) just so we can catch any case where this
> accidentally gets called more than once?

I figured the null pointer dereference on 616 would be a strong enough signal!

https://codereview.chromium.org/14217008/diff/16002/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:627: return audio_.get() ? audio_.get() :
disabled_audio_.get();
On 2013/04/24 06:09:57, acolwell wrote:
> Is this change actually needed? Doesn't this change the existing behavior when
> OnAudioRendererDisabled() is called.

Done + updated unit test

[commit-bot: I haz the power, 2013-04-25 00:13:21]

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

[commit-bot: I haz the power, 2013-04-25 00:13:38]

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

Patch:       media/filters/audio_decoder_selector.cc
Index: media/filters/audio_decoder_selector.cc
diff --git a/media/filters/audio_decoder_selector.cc
b/media/filters/audio_decoder_selector.cc
index
1cc9fbf9e40771032626cbbe6ed682bf2967935c..1195ca154a0956c138adb86ac888adc50fc59ff0
100644
--- a/media/filters/audio_decoder_selector.cc
+++ b/media/filters/audio_decoder_selector.cc
@@ -24,13 +24,14 @@ AudioDecoderSelector::AudioDecoderSelector(
     : message_loop_(message_loop),
       decoders_(decoders.Pass()),
       set_decryptor_ready_cb_(set_decryptor_ready_cb),
-      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+      input_stream_(NULL),
+      weak_ptr_factory_(this) {
 }
 
 AudioDecoderSelector::~AudioDecoderSelector() {}
 
 void AudioDecoderSelector::SelectAudioDecoder(
-    const scoped_refptr<DemuxerStream>& stream,
+    DemuxerStream* stream,
     const StatisticsCB& statistics_cb,
     const SelectDecoderCB& select_decoder_cb) {
   DVLOG(2) << "SelectAudioDecoder()";
@@ -44,7 +45,7 @@ void AudioDecoderSelector::SelectAudioDecoder(
   if (!config.IsValidConfig()) {
     DLOG(ERROR) << "Invalid audio stream config.";
     base::ResetAndReturn(&select_decoder_cb_).Run(
-        scoped_ptr<AudioDecoder>(), NULL);
+        scoped_ptr<AudioDecoder>(), scoped_ptr<DecryptingDemuxerStream>());
     return;
   }
 
@@ -59,7 +60,7 @@ void AudioDecoderSelector::SelectAudioDecoder(
   // This could happen if Encrypted Media Extension (EME) is not enabled.
   if (set_decryptor_ready_cb_.is_null()) {
     base::ResetAndReturn(&select_decoder_cb_).Run(
-        scoped_ptr<AudioDecoder>(), NULL);
+        scoped_ptr<AudioDecoder>(), scoped_ptr<DecryptingDemuxerStream>());
     return;
   }
 
@@ -79,14 +80,15 @@ void AudioDecoderSelector::DecryptingAudioDecoderInitDone(
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (status == PIPELINE_OK) {
-    base::ResetAndReturn(&select_decoder_cb_).Run(audio_decoder_.Pass(), NULL);
+    base::ResetAndReturn(&select_decoder_cb_).Run(
+        audio_decoder_.Pass(), scoped_ptr<DecryptingDemuxerStream>());
     return;
   }
 
   audio_decoder_.reset();
 
-  decrypted_stream_ = new DecryptingDemuxerStream(
-      message_loop_, set_decryptor_ready_cb_);
+  decrypted_stream_.reset(new DecryptingDemuxerStream(
+      message_loop_, set_decryptor_ready_cb_));
 
   decrypted_stream_->Initialize(
       input_stream_,
@@ -100,14 +102,14 @@ void
AudioDecoderSelector::DecryptingDemuxerStreamInitDone(
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (status != PIPELINE_OK) {
-    decrypted_stream_ = NULL;
+    decrypted_stream_.reset();
     base::ResetAndReturn(&select_decoder_cb_).Run(
-        scoped_ptr<AudioDecoder>(), NULL);
+        scoped_ptr<AudioDecoder>(), scoped_ptr<DecryptingDemuxerStream>());
     return;
   }
 
   DCHECK(!decrypted_stream_->audio_decoder_config().is_encrypted());
-  input_stream_ = decrypted_stream_;
+  input_stream_ = decrypted_stream_.get();
   InitializeDecoder(decoders_.begin());
 }
 
@@ -117,7 +119,7 @@ void AudioDecoderSelector::InitializeDecoder(
 
   if (iter == decoders_.end()) {
     base::ResetAndReturn(&select_decoder_cb_).Run(
-        scoped_ptr<AudioDecoder>(), NULL);
+        scoped_ptr<AudioDecoder>(), scoped_ptr<DecryptingDemuxerStream>());
     return;
   }
 
@@ -143,7 +145,7 @@ void AudioDecoderSelector::DecoderInitDone(
   decoders_.weak_erase(iter);
 
   base::ResetAndReturn(&select_decoder_cb_).Run(audio_decoder.Pass(),
-                                                decrypted_stream_);
+                                                decrypted_stream_.Pass());
 }
 
 }  // namespace media

[commit-bot: I haz the power, 2013-04-25 06:49:29]

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

[commit-bot: I haz the power, 2013-04-25 08:51:48]

Change committed as 196365