Rename VideoDecoder::HasOutputFrameAvailable() to CanReadWithoutStalling().

Rename VideoDecoder::HasOutputFrameAvailable() to CanReadWithoutStalling().

Also fix the implementation in GpuVideoDecoder to better reflect what we are
checking.

BUG=256899, 141788
TEST=Tested on Windows.
R=acolwell@chromium.org, scherkus@chromium.org

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

[xhwang, 2013-07-03 00:54:40]

PTAL

[scherkus (not reviewing), 2013-07-03 01:04:11]

guessing a test is too difficult given it's GVD?

[xhwang, 2013-07-03 02:50:25]

On 2013/07/03 01:04:11, scherkus wrote:
> guessing a test is too difficult given it's GVD?

No GVD test! Since this is actually more like a VBR issue, I added a VBR test.
PTAL.

For now I am just trying to land my media pipeline refactoring CL. So I am
thinking of landing this first then fix VBR later. WDYT?

[acolwell GONE FROM CHROMIUM, 2013-07-03 16:43:56]

lgtm % comment

https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:530: TEST_F(VideoRendererBaseTest,
DISABLED_PlayAfterPrematurePreroll) {
Why is this disabled? Shouldn't the fix make this pass now? If not, then I don't
think we should add the test until we have an actual fix.

[xhwang, 2013-07-03 17:51:44]

https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:530: TEST_F(VideoRendererBaseTest,
DISABLED_PlayAfterPrematurePreroll) {
On 2013/07/03 16:43:56, acolwell wrote:
> Why is this disabled? Shouldn't the fix make this pass now? If not, then I
don't
> think we should add the test until we have an actual fix.

The root issue is that VRB preroll could end up with no prerolled frames after
preroll is finished, due to this CanDecodeMore() call/check. In that case
playback may stall. I think we need to make it clear about the contract b/w VRB
and the decoder: (1) Does VRB always expect at least one frame with
ts>=preroll_ts when preroll is finished? Or, (2) should VRB be prepared for this
to happen and has a way to get out of the stalled state (e.g. issuing more
reads). 

I feel the (1) makes more sense to me. The fix in GVD follows that.

The test added here has nothing to do with GVD; it uses the MockVideoDecoder. I
added this test only to show what I saw (how to repro the issue). If we all
agree with (1) then this should never happen and we could probably just drop
this test.

[acolwell GONE FROM CHROMIUM, 2013-07-03 18:17:59]

On 2013/07/03 17:51:44, xhwang wrote:
>
https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
> File media/filters/video_renderer_base_unittest.cc (right):
> 
>
https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
> media/filters/video_renderer_base_unittest.cc:530:
TEST_F(VideoRendererBaseTest,
> DISABLED_PlayAfterPrematurePreroll) {
> On 2013/07/03 16:43:56, acolwell wrote:
> > Why is this disabled? Shouldn't the fix make this pass now? If not, then I
> don't
> > think we should add the test until we have an actual fix.
> 
> The root issue is that VRB preroll could end up with no prerolled frames after
> preroll is finished, due to this CanDecodeMore() call/check. In that case
> playback may stall. I think we need to make it clear about the contract b/w
VRB
> and the decoder: (1) Does VRB always expect at least one frame with
> ts>=preroll_ts when preroll is finished? Or, (2) should VRB be prepared for
this
> to happen and has a way to get out of the stalled state (e.g. issuing more
> reads). 
> 
> I feel the (1) makes more sense to me. The fix in GVD follows that.
> 
> The test added here has nothing to do with GVD; it uses the MockVideoDecoder.
I
> added this test only to show what I saw (how to repro the issue). If we all
> agree with (1) then this should never happen and we could probably just drop
> this test.

I agree with (1). VRB should expect to get at least 1 frame during preroll
unless preroll_ts happens to be just beyond the last frame and EoS is returned
from the read instead. As long as we aren't introducing a way for leaked frames
to get masked by early preroll returns, I think we'll be fine.

[scherkus (not reviewing), 2013-07-03 20:26:49]

On 2013/07/03 18:17:59, acolwell wrote:
> On 2013/07/03 17:51:44, xhwang wrote:
> >
>
https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
> > File media/filters/video_renderer_base_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/18580008/diff/4001/media/filters/video_render...
> > media/filters/video_renderer_base_unittest.cc:530:
> TEST_F(VideoRendererBaseTest,
> > DISABLED_PlayAfterPrematurePreroll) {
> > On 2013/07/03 16:43:56, acolwell wrote:
> > > Why is this disabled? Shouldn't the fix make this pass now? If not, then I
> > don't
> > > think we should add the test until we have an actual fix.
> > 
> > The root issue is that VRB preroll could end up with no prerolled frames
after
> > preroll is finished, due to this CanDecodeMore() call/check. In that case
> > playback may stall. I think we need to make it clear about the contract b/w
> VRB
> > and the decoder: (1) Does VRB always expect at least one frame with
> > ts>=preroll_ts when preroll is finished? Or, (2) should VRB be prepared for
> this
> > to happen and has a way to get out of the stalled state (e.g. issuing more
> > reads). 
> > 
> > I feel the (1) makes more sense to me. The fix in GVD follows that.
> > 
> > The test added here has nothing to do with GVD; it uses the
MockVideoDecoder.
> I
> > added this test only to show what I saw (how to repro the issue). If we all
> > agree with (1) then this should never happen and we could probably just drop
> > this test.
> 
> I agree with (1). VRB should expect to get at least 1 frame during preroll
> unless preroll_ts happens to be just beyond the last frame and EoS is returned
> from the read instead. As long as we aren't introducing a way for leaked
frames
> to get masked by early preroll returns, I think we'll be fine.

+1 to acolwell / (1)

lgtm

[xhwang, 2013-07-03 20:33:47]

Thanks! Then I'll remove the disabled test.

Any preference on naming? fischman@ proposed AbleToReadWithoutStalling(). I feel
it's only slightly better than CanDecodeMore() and it's longer. WDYT?

[acolwell GONE FROM CHROMIUM, 2013-07-03 20:42:55]

On 2013/07/03 20:33:47, xhwang wrote:
> Thanks! Then I'll remove the disabled test.
> 
> Any preference on naming? fischman@ proposed AbleToReadWithoutStalling(). I
feel
> it's only slightly better than CanDecodeMore() and it's longer. WDYT?

I like the longer name because it does more accurately indicate what is going
on.

[scherkus (not reviewing), 2013-07-03 20:53:11]

On 2013/07/03 20:42:55, acolwell wrote:
> On 2013/07/03 20:33:47, xhwang wrote:
> > Thanks! Then I'll remove the disabled test.
> > 
> > Any preference on naming? fischman@ proposed AbleToReadWithoutStalling(). I
> feel
> > it's only slightly better than CanDecodeMore() and it's longer. WDYT?
> 
> I like the longer name because it does more accurately indicate what is going
> on.

+1

AFAIK that's why we went with HasOutputFrameAvailable() as opposed to something
like CanDecodeMore() in the first place

[xhwang, 2013-07-03 21:08:26]

On 2013/07/03 20:53:11, scherkus wrote:
> On 2013/07/03 20:42:55, acolwell wrote:
> > On 2013/07/03 20:33:47, xhwang wrote:
> > > Thanks! Then I'll remove the disabled test.
> > > 
> > > Any preference on naming? fischman@ proposed AbleToReadWithoutStalling().
I
> > feel
> > > it's only slightly better than CanDecodeMore() and it's longer. WDYT?
> > 
> > I like the longer name because it does more accurately indicate what is
going
> > on.
> 
> +1
> 
> AFAIK that's why we went with HasOutputFrameAvailable() as opposed to
something
> like CanDecodeMore() in the first place

Done.

[commit-bot: I haz the power, 2013-07-03 21:34:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/18580008/17001

[xhwang, 2013-07-03 23:30:28]

Committed patchset #4 manually as r210066 (presubmit successful).