Don't try to decode more than one EOS at a time.

Don't try to decode more than one EOS at a time.

BUG=405642
Committed: https://crrev.com/1c8af4fa3f7fd28d69958c40795b2ddebeec3335
Cr-Commit-Position: refs/heads/master@{#292268}

[xhwang, 2014-08-21 16:37:07]

Looking pretty good. Just a few nits/comments. Also, can you add a unit test for
this? You can add the test in video_frame_stream_unittest.cc.

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream.cc
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.cc:281: bool end_of_stream,
Since we have "decoding_eos_", this parameter seems redundant. We can probably
just check "decoding_eos_" at l.326.

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream.h
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.h:187: // be sent for decoding until the resulting
flush completes.
nit: s/until .../unless the decoder is reset or reinitialized/

[sandersd (OOO until July 31), 2014-08-23 00:49:15]

I've added a unittest, but Reset_AfterEndOfStream is failing now and I've not
figured out why yet.

Note that after my change to fake_video_decoder.cc, several of the existing
tests failed without my fix; the new test case isn't really testing new
behavior.

Since the actual new check was added in fake_video_decoder.cc, should that be a
CHECK instead, or do I need to find a way to convert the DCHECK into an EXPECT
somehow?

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream.cc
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.cc:281: bool end_of_stream,
On 2014/08/21 16:37:07, xhwang wrote:
> Since we have "decoding_eos_", this parameter seems redundant. We can probably
> just check "decoding_eos_" at l.326.

Since there can be multiple pending decodes, I don't believe they are the same.

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream.h
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.h:187: // be sent for decoding until the resulting
flush completes.
On 2014/08/21 16:37:07, xhwang wrote:
> nit: s/until .../unless the decoder is reset or reinitialized/

Updated to reflect the final logic I implemented.

[sandersd (OOO until July 31), 2014-08-25 18:12:38]

I modified |FakeVideoDecode| to leave the end of stream state after it satisfies
the decode request. This fixes all the tests that broke when I added the DCHECK.

[sandersd (OOO until July 31), 2014-08-26 17:43:04]

sandersd@chromium.org changed reviewers:
+ rileya@chromium.org
- xhwang@chromium.org

[sandersd (OOO until July 31), 2014-08-26 17:43:04]

Reassigning to riley as xhwang is OOO.

[rileya (GONE FROM CHROMIUM), 2014-08-26 22:36:29]

On 2014/08/26 17:43:04, sandersd wrote:
> Reassigning to riley as xhwang is OOO.

lgtm % comment (I'm not an OWNER though, so someone else will need to take a
peek)

Is this behavior (not decoding multiple EOSs at once) documented somewhere? If
not, it might be a good idea to mention the bug somewhere. Maybe note in GVD
that it chokes on this, or mention it as justification at the declaration of
|decoding_eos_| or something like that.

Iirc there isn't anything saying not to pass multiple EOSs at once to a Decoder
anywhere, so it might not be clear to someone in the future why we're enforcing
this particular behavior here.

[sandersd (OOO until July 31), 2014-08-26 23:09:53]

> Is this behavior (not decoding multiple EOSs at once) documented somewhere? If
> not, it might be a good idea to mention the bug somewhere. Maybe note in GVD
> that it chokes on this, or mention it as justification at the declaration of
> |decoding_eos_| or something like that.

I can't find anything, but |DecoderStream| itself will fail with a DCHECK if a
decode completes once a previous end of stream decode has completed.

I've added a comment to video_decoder.h, guaranteeing this behavior.

[sandersd (OOO until July 31), 2014-08-26 23:11:10]

sandersd@chromium.org changed reviewers:
+ acolwell@chromium.org

[sandersd (OOO until July 31), 2014-08-26 23:11:10]

acolwell@: for OWNERS approval.

[acolwell GONE FROM CHROMIUM, 2014-08-27 00:44:39]

looks good. Just a few minor questions.

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
media/filters/decoder_stream.cc:270: if (buffer->end_of_stream())
nit: Shouldn't this be before the Decode() call just in case the callback is
called synchronously inside Decode()?

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
media/filters/decoder_stream.cc:294: decoding_eos_ = false;
Is there any reason not to simply make this a STATE_ instead of an extra member
variable? It seems like this could be modeled as the state between NORMAL and
END_OF_STREAM.

[sandersd (OOO until July 31), 2014-08-27 01:13:46]

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
media/filters/decoder_stream.cc:270: if (buffer->end_of_stream())
On 2014/08/27 00:44:39, acolwell wrote:
> nit: Shouldn't this be before the Decode() call just in case the callback is
> called synchronously inside Decode()?

Decode() is defined not to do that, but seems like a good idea.

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
media/filters/decoder_stream.cc:294: decoding_eos_ = false;
On 2014/08/27 00:44:39, acolwell wrote:
> Is there any reason not to simply make this a STATE_ instead of an extra
member
> variable? It seems like this could be modeled as the state between NORMAL and
> END_OF_STREAM.

This is true, but I was having trouble proving that change correct when taking
reinitialization (which also sends EOS buffers) and resets into account. It was
conceptually simpler to track the state of the decoder.

I'm willing to take on the task of documenting the whole state machine well
enough to be certain of such changes, if that will be useful to others as well.

[acolwell GONE FROM CHROMIUM, 2014-08-27 01:49:47]

lgtm

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
media/filters/decoder_stream.cc:294: decoding_eos_ = false;
On 2014/08/27 01:13:45, sandersd wrote:
> On 2014/08/27 00:44:39, acolwell wrote:
> > Is there any reason not to simply make this a STATE_ instead of an extra
> member
> > variable? It seems like this could be modeled as the state between NORMAL
and
> > END_OF_STREAM.
> 
> This is true, but I was having trouble proving that change correct when taking
> reinitialization (which also sends EOS buffers) and resets into account. It
was
> conceptually simpler to track the state of the decoder.
> 
> I'm willing to take on the task of documenting the whole state machine well
> enough to be certain of such changes, if that will be useful to others as
well.

OK. I don't think it is worth holding things up right now. Perhaps file a
cleanup bug and put TODO for documenting the state machine and potentially
removing the decoding_eos_ var.

[sandersd (OOO until July 31), 2014-08-27 21:45:59]

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/490033002/diff/80001/media/filters/decoder_st...
media/filters/decoder_stream.cc:294: decoding_eos_ = false;
On 2014/08/27 01:49:47, acolwell wrote:
> On 2014/08/27 01:13:45, sandersd wrote:
> > On 2014/08/27 00:44:39, acolwell wrote:
> > > Is there any reason not to simply make this a STATE_ instead of an extra
> > member
> > > variable? It seems like this could be modeled as the state between NORMAL
> and
> > > END_OF_STREAM.
> > 
> > This is true, but I was having trouble proving that change correct when
taking
> > reinitialization (which also sends EOS buffers) and resets into account. It
> was
> > conceptually simpler to track the state of the decoder.
> > 
> > I'm willing to take on the task of documenting the whole state machine well
> > enough to be certain of such changes, if that will be useful to others as
> well.
> 
> OK. I don't think it is worth holding things up right now. Perhaps file a
> cleanup bug and put TODO for documenting the state machine and potentially
> removing the decoding_eos_ var.
> 
> 

Done.

[sandersd (OOO until July 31), 2014-08-27 21:46:19]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-08-27 21:48:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sandersd@chromium.org/490033002/120001

[xhwang, 2014-08-27 22:32:51]

xhwang@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2014-08-27 22:32:51]

lgtm

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream.cc
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/490033002/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.cc:281: bool end_of_stream,
On 2014/08/23 00:49:15, sandersd wrote:
> On 2014/08/21 16:37:07, xhwang wrote:
> > Since we have "decoding_eos_", this parameter seems redundant. We can
probably
> > just check "decoding_eos_" at l.326.
> 
> Since there can be multiple pending decodes, I don't believe they are the
same.

Got it. Thanks for the clarification.

https://codereview.chromium.org/490033002/diff/120001/media/filters/video_fra...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/490033002/diff/120001/media/filters/video_fra...
media/filters/video_frame_stream_unittest.cc:495: if
(GetParam().parallel_decoding == 1 || GetParam().decoding_delay != 0)
For the record, we don't run the test when we have decoding delay since we would
have triggered EOS decoding (flushing) after the for loop block of 501-512.

[commit-bot: I haz the power, 2014-08-27 22:54:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-27 23:43:54]

Committed patchset #7 (id:120001) as 3e7a876c7d7e91be1e0bdc271d7096b7ee78fd7a

[commit-bot: I haz the power, 2014-09-10 02:55:39]

Patchset 7 (id:??) landed as
https://crrev.com/1c8af4fa3f7fd28d69958c40795b2ddebeec3335
Cr-Commit-Position: refs/heads/master@{#292268}