A CL to remove the two tests and only keep the (!drop_frames_) test would be rather trivial and I can prepare it, but I'm not 100% sure whether what I'm seeing in VideoRendererImplTest.UnderflowEvictionWhileHaveEnough is not some limitation of the testing framework.

The RemoveFramesForUnderflowOrBackgroundRendering() call that is supposed to make the transition to BUFFERING_HAVE_NOTHING happens while |was_background_rendering_| is false due to the renderer_->OnTimeStopped() call that precedes the waiting for the transition.

Sorry, I'm unclear on what the issue is here. This code should only return early when framed dropping is enabled and it's definitely not enabled for all tests when they are actually testing some aspect of frame dropping.

Can you elaborate on exactly what's wrong?

This is the change I'm thinking about making:

 void VideoRendererImpl::RemoveFramesForUnderflowOrBackgroundRendering() {
-  // Nothing to do if we're not underflowing, background rendering, or frame
-  // dropping is disabled (test only).
-  const bool have_nothing = buffering_state_ == BUFFERING_HAVE_NOTHING;
-  if (!was_background_rendering_ && !have_nothing && !drop_frames_)
+  // Nothing to do if frame dropping is disabled (test only).
+  if (!drop_frames_)
     return;

This allows entrance into this function for non-test code in new situations; running it during have_enough is not correct.

I realize now I wasn't clear at all in my original description, sorry about that :)

> This code should only return early when framed dropping is enabled

I'm probably getting mixed up in the various conditions, but I'm honestly not sure if you actually meant "disabled" here, because currently this code only returns early if frame dropping is disabled :)

While running VideoRendererImplTest.UnderflowEvictionWhileHaveEnough, I noticed that, shortly before entering the wait for BUFFERING_HAVE_NOTHING, the test clears |was_background_rendering_| through the call to renderer_->OnTimeStopped().  This would allow the next RemoveFramesForUnderflowOrBackgroundRendering() to return early, and the only thing preventing the early return is |drop_frames_| equal to true.  So it's just the "if (!drop_frames_)" test that makes the TransitionToHaveNothing_Locked() call added to RemoveFramesForUnderflowOrBackgroundRendering() in https://codereview.chromium.org/2363983003 effective.

Thus, it seems, there are two possibilities:
  - RemoveFramesForUnderflowOrBackgroundRendering() should call TransitionToHaveNothing_Locked() even if |was_background_rendering_| is false.
  - The clearing of |was_background_rendering_| in VideoRendererImplTest.UnderflowEvictionWhileHaveEnough is not a realistic scenario.

As per c#6, this is related to video rendering, give to dale to start with.

I think wdzierzanowski@ fixed this?

Sorry, no, I did not.  There was no follow-up to #c6 and I guess that's because it's still too vague.

I think a part of the reason could be me struggling to understanding the idea behind the beginning of the function VideoRendererImpl::RemoveFramesForUnderflowOrBackgroundRendering(), so bear with me please:

  // Nothing to do if we're not underflowing, background rendering, or frame
  // dropping is disabled (test only).
  const bool have_nothing = buffering_state_ == BUFFERING_HAVE_NOTHING;
  if (!was_background_rendering_ && !have_nothing && !drop_frames_)
    return;

For one, the condition seems to say something different than the comment (there is one explicit "or" that doesn't fit, and possibly another, implicit one).

And then there's the thing about |drop_frames_|.  Frame dropping is only disabled in certain test scenarios.  In all other cases (production and those tests that don't disable frame dropping), we run the full RemoveFramesForUnderflowOrBackgroundRendering() -- regardless of whether we're currently underflowing or background rendering.  Is this the intended behavior?

I have a feeling the discussion might get easier once we clarify this part :)

Ah, I see your point now. It looks like we might be expiring some frames in situations we shouldn't, which shouldn't do much other than effect smoothness. Will take a look.

https://codereview.chromium.org/2570013003 should resolve this. Sorry I wasn't parsing the conditional correctly in my head. You're right that it's currently incorrect, though I wasn't able to reproduce any issues; I expect they would only occur with very-fast decoders and playbacks that were already on the edge of badness.

Thanks, RemoveFramesForUnderflowOrBackgroundRendering() is much clearer (and apparently more correct) with this CL :)

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

commit 8ac9ef71a561a281bd867e23fcd31e129373ab37
Author: dalecurtis <dalecurtis@chromium.org>
Date: Wed Dec 14 19:39:12 2016

Fix underflow correction occurring during normal rendering.

We should not be trying to expire frames during the normal
rendering process. This corrects a conditional which was
incorrectly allowing this behavior and fixes the test which
were relying on this behavior.

Testing the inverse (i.e. what was broken) is not in this
CL, since I ran out of time to add it for now.

Notably, I wasn't able to reproduce any issues arising from
this mistake. In practice, Render() occurs with such frequency
that it's rare for FrameReady() to occur fast enough to out
pace it in a meaningful manner relative to the frame duration.

BUG= 667704 
TEST=updated unittests.

Review-Url: https://codereview.chromium.org/2570013003
Cr-Commit-Position: refs/heads/master@{#438596}

[modify] https://crrev.com/8ac9ef71a561a281bd867e23fcd31e129373ab37/media/renderers/video_renderer_impl.cc
[modify] https://crrev.com/8ac9ef71a561a281bd867e23fcd31e129373ab37/media/renderers/video_renderer_impl.h
[modify] https://crrev.com/8ac9ef71a561a281bd867e23fcd31e129373ab37/media/renderers/video_renderer_impl_unittest.cc

Thanks for pushing on this!