Fix DCHECK when track status changes in non-playing state

media/renderers/renderer_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/renderers/renderer_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 211 matching lines @@
 }
 
 void RendererImpl::RestartStreamPlayback(DemuxerStream* stream,
                                          bool enabled,
                                          base::TimeDelta time) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(stream);
   bool video = (stream->type() == DemuxerStream::VIDEO);
   DVLOG(1) << __func__ << (video ? " video" : " audio") << " stream=" << stream
            << " enabled=" << stream->enabled() << " time=" << time.InSecondsF();
-  if (state_ != STATE_PLAYING)
+  if ((state_ != STATE_PLAYING) || (audio_ended_ && video_ended_))

[chcunningham, 2016/08/24 22:16:53]

This seems to be a separate issue from relaxing that DCHECK? Can you explain the
circumstances here?

[servolk, 2016/08/24 22:46:32]

Yes, somewhat separate, but related. First I've simply updated the DCHECK
condition (see patchset #1). But then I decided to add a unit test that would
test changing media track status while the pipeline is not in the kPlaying state
(previously it would trip on that DCHECK). But when I added a unit test that
waits for playback to end (so that pipeline is in the stopped state) and then
changes tracks, it would hit a different DCHECK (here
https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=0&l=552).
It turns out that RendererImpl stays in the STATE_PLAYING even after both
renderers are ended. So we would get here and would try to call
RestartA/VRenderer below even though the playback of both streams has already
ended. By adding this additional check we'll ensure that media track status
changes when the RendererImpl has been ended, we won't try to restart renderers.
Alternatively I guess we could try to set RendererImpl::state_ to something else
besides STATE_PLAYING in RendererImpl::RunEndedCallbackIfNeeded when we detect
that playback has actually ended. But none of the other RendererImpl::State enum
values seem to be fitting this situation, and I wanted to keep this change small
and safe. So I've chose to modify the check here, rather than mess with state_
or introduce a new State enum value.

     return;
   if (stream->type() == DemuxerStream::VIDEO) {
     DCHECK(video_renderer_);
     if (restarting_video_)
       return;
     restarting_video_ = true;
     video_renderer_->Flush(
         base::Bind(&RendererImpl::RestartVideoRenderer, weak_this_, time));
   } else if (stream->type() == DemuxerStream::AUDIO) {
     DCHECK(audio_renderer_);
@@ skipping 606 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoNaturalSizeChange(size);
 }
 
 void RendererImpl::OnVideoOpacityChange(bool opaque) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoOpacityChange(opaque);
 }
 
 }  // namespace media