Report PIPELINE_ERROR_DECODE when SourceState::Append fails.

Report PIPELINE_ERROR_DECODE when SourceState::Append fails.

In old implementaion, when SourceState::Append failed, it will report
DEMUXER_ERROR_COULD_NOT_OPEN error in ChunkDemuxer::AppendData case INITIALIZING.
But in fact, appended data may include initialization segment and media segment both.
The error can happen in any point.
In MSE spec
( http://w3c.github.io/media-source/#byte-stream-formats )
The user agent must run the end of stream algorithm with the error parameter
set to "decode" when in some conditions. They all happen in parsing the appended data.
So, here when SourceState::Append fails, it should report PIPELINE_ERROR_DECODE.

Another side, when the segment parser loop successfully parses a complete
initialization segment, it should call ReportMetadata immediately. Otherwise,
when meeting PIPELINE_ERROR_DECODE but WebMediaPlayer's ready_state_ is still
ReadyStateHaveNothing, it will be treated as NetworkStateFormatError not
NetworkStateDecodeError.

This Cl will resolve above two problems.

BUG=None
TEST=LayoutTest: https://codereview.chromium.org/742653002

Committed: https://crrev.com/5025fd8f7f15225b454ad37bdec8e696db4330f3
Cr-Commit-Position: refs/heads/master@{#313358}

[jiajia.qin, 2014-11-12 10:55:30]

PTAL.

[jiajia.qin, 2014-11-17 05:31:14]

Hi, wolenetz. Any feedback? Thanks.

[jiajia.qin, 2014-11-18 07:55:29]

jiajia.qin@intel.com changed reviewers:
+ dalecurtis@chromium.org, philipj@opera.com, xhwang@chromium.org

[jiajia.qin, 2014-11-18 07:55:30]

Add more reviewers. Please take a look. Thanks.

[philipj_slow, 2014-11-18 09:24:23]

Note that I'm not an owner in Chromium, but that's OK.

Can you include a link to the relevant section of the MSE spec in the commit
message, since you refer to it?

It seems to me like the ReportMetadata() change is independent of the
PIPELINE_ERROR_DECODE change, is there a dependency between them that I'm
missing?

Finally, a test case that exposes the difference in behavior would be good. It
probably makes most sense to write it as a LayoutTest, which would turn this
into a 3-sided patch, but such is life until the repos merge :/

P.S. Can you keep the commit message to the 72-80 character limit? It's kind of
hard to read in the Web interface, and will be even harder in a narrow terminal.

https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:304: ReportMetadata();
Will this affect both MSE and normal playback? The ReportMetadata() for the
kPlaying case looks odd and this more sane, but will this affect when e.g. the
loadedmetadata event is fired? Do all LayoutTests still pass?

[xhwang, 2014-11-18 17:19:52]

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

[xhwang, 2014-11-18 17:19:52]

I'll defer to wolenetz@ to review this one.

[DaleCurtis, 2014-11-19 00:20:29]

https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:303: if (state_ == kInitDemuxer && status == PIPELINE_OK)
{
Why here instead of in StateTransitionTask before initializing the renderer?

[jiajia.qin, 2014-11-19 06:30:07]

On 2014/11/18 09:24:23, philipj wrote:
> Note that I'm not an owner in Chromium, but that's OK.
> 
> Can you include a link to the relevant section of the MSE spec in the commit
> message, since you refer to it?
Done

> It seems to me like the ReportMetadata() change is independent of the
> PIPELINE_ERROR_DECODE change, is there a dependency between them that I'm
> missing?
ReportMetadata is only called by here. I can't find other places to use it.
Here, when demuxer_ has finished parsing the initialization segment,
Pipeline::OnStateTransition will be executed. According
to(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-init-segment-received)
step 6.2, we should call
ReportMetadata so that HTMLMediaElement can set readyState attribute to
HAVE_METADATA. 
  
> Finally, a test case that exposes the difference in behavior would be good. It
> probably makes most sense to write it as a LayoutTest, which would turn this
> into a 3-sided patch, but such is life until the repos merge :/
Done. See https://codereview.chromium.org/742653002.

> P.S. Can you keep the commit message to the 72-80 character limit? It's kind
of
> hard to read in the Web interface, and will be even harder in a narrow
terminal.
Done

> https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc
> File media/base/pipeline.cc (right):
> 
>
https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
> media/base/pipeline.cc:304: ReportMetadata();
> Will this affect both MSE and normal playback? The ReportMetadata() for the
> kPlaying case looks odd and this more sane, but will this affect when e.g. the
> loadedmetadata event is fired? Do all LayoutTests still pass?
I think it won't affect loadedmetadata event. In my side, I haven't noticed any
regression.

[jiajia.qin, 2014-11-19 06:42:44]

On 2014/11/19 00:20:29, DaleCurtis wrote:
> https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc
> File media/base/pipeline.cc (right):
> 
>
https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
> media/base/pipeline.cc:303: if (state_ == kInitDemuxer && status ==
PIPELINE_OK)
> {
> Why here instead of in StateTransitionTask before initializing the renderer?
I think there is no need to put it after the renderer. When demuxer has been
initialized and
StateTransitionTask is called, it means the parser has successfully parses a
complete initialization segment.
At this point, we can set HTMLMediaElement.readyState attribute to
HAVE_METADATA. But if we defer this process,
the parser maybe has begin to parse media segment and meet 'decode' error as
what I said in the issue description.

[philipj_slow, 2014-11-19 10:20:58]

On 2014/11/19 06:30:07, jiajia.qin wrote:
> On 2014/11/18 09:24:23, philipj wrote:
> > Note that I'm not an owner in Chromium, but that's OK.
> > 
> > Can you include a link to the relevant section of the MSE spec in the commit
> > message, since you refer to it?
> Done

On a new line or something so that it's recognized as a link and clickable :)

> > It seems to me like the ReportMetadata() change is independent of the
> > PIPELINE_ERROR_DECODE change, is there a dependency between them that I'm
> > missing?
> ReportMetadata is only called by here. I can't find other places to use it.
> Here, when demuxer_ has finished parsing the initialization segment,
> Pipeline::OnStateTransition will be executed. According
>
to(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-init-segment-received)
> step 6.2, we should call
> ReportMetadata so that HTMLMediaElement can set readyState attribute to
> HAVE_METADATA. 

OK, so without this change, appending only an initialization segment would not
cause the HTMLMediaElement to reach HAVE_METADATA? That sounds like a separate
bug, if so can you separate it into a separate CL with its own test?

> > Finally, a test case that exposes the difference in behavior would be good.
It
> > probably makes most sense to write it as a LayoutTest, which would turn this
> > into a 3-sided patch, but such is life until the repos merge :/
> Done. See https://codereview.chromium.org/742653002.

Thanks!

> > https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc
> > File media/base/pipeline.cc (right):
> > 
> >
>
https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
> > media/base/pipeline.cc:304: ReportMetadata();
> > Will this affect both MSE and normal playback? The ReportMetadata() for the
> > kPlaying case looks odd and this more sane, but will this affect when e.g.
the
> > loadedmetadata event is fired? Do all LayoutTests still pass?
> I think it won't affect loadedmetadata event. In my side, I haven't noticed
any
> regression.

That seems odd, if I understand kPlaying correctly it's when everything is
prerolled and ready to play, even if not actually playing. That should be much
later than simply successfully initializing the demuxer. Is the init segment
enough to set up the decoders as well? I'm assuming that's needed to know the
video size for certain? If so, is what pipeline.h documents as 'When all filter
initialization states have completed, we are implicitly in a "Paused" state.'
the time to report metadata?

[jiajia.qin, 2014-11-20 06:49:19]

On 2014/11/19 10:20:58, philipj wrote:
> On 2014/11/19 06:30:07, jiajia.qin wrote:
> > On 2014/11/18 09:24:23, philipj wrote:
> > > Note that I'm not an owner in Chromium, but that's OK.
> > > 
> > > Can you include a link to the relevant section of the MSE spec in the
commit
> > > message, since you refer to it?
> > Done
> 
> On a new line or something so that it's recognized as a link and clickable :)
> 
Done

> > > It seems to me like the ReportMetadata() change is independent of the
> > > PIPELINE_ERROR_DECODE change, is there a dependency between them that I'm
> > > missing?
> > ReportMetadata is only called by here. I can't find other places to use it.
> > Here, when demuxer_ has finished parsing the initialization segment,
> > Pipeline::OnStateTransition will be executed. According
> >
>
to(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-init-segment-received)
> > step 6.2, we should call
> > ReportMetadata so that HTMLMediaElement can set readyState attribute to
> > HAVE_METADATA. 
> 
> OK, so without this change, appending only an initialization segment would not
> cause the HTMLMediaElement to reach HAVE_METADATA? That sounds like a separate
> bug, if so can you separate it into a separate CL with its own test?
> 
Without this change, appending only an initialization segment would also cause
HTMLMediaElement.readyState to reach HAVE_METADATA.
But it will happen in a delay(it will have to wait until renderer_ is
initialized.
But during this, parser maybe have parsed some media data).
I do this change is to reduce this delay.


> > > Finally, a test case that exposes the difference in behavior would be
good.
> It
> > > probably makes most sense to write it as a LayoutTest, which would turn
this
> > > into a 3-sided patch, but such is life until the repos merge :/
> > Done. See https://codereview.chromium.org/742653002.
> 
> Thanks!
> 
> > > https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc
> > > File media/base/pipeline.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
> > > media/base/pipeline.cc:304: ReportMetadata();
> > > Will this affect both MSE and normal playback? The ReportMetadata() for
the
> > > kPlaying case looks odd and this more sane, but will this affect when e.g.
> the
> > > loadedmetadata event is fired? Do all LayoutTests still pass?
> > I think it won't affect loadedmetadata event. In my side, I haven't noticed
> any
> > regression.
> 
> That seems odd, if I understand kPlaying correctly it's when everything is
> prerolled and ready to play, even if not actually playing. That should be much
> later than simply successfully initializing the demuxer. Is the init segment
> enough to set up the decoders as well? I'm assuming that's needed to know the
> video size for certain? If so, is what pipeline.h documents as 'When all
filter
> initialization states have completed, we are implicitly in a "Paused" state.'
> the time to report metadata?
I think the init segment is enough to set up the decoders (When parsing the init
segment,
we can get all info to config the DemuxerStream. Then, it can be used to
initialize the renderer_
which will take charge to initialize the decoder). 

I agree with you about kPlaying's meaning (when pipeline state is kPlaying,
it means all init/config work is done including demuxer, renderer, decoder.
But it hasn't begin to decode any data).
I think it doesn't conflict. I checked HAVE_METADATA's definition.
( http://www.w3.org/TR/html5/embedded-content-0.html#dom-media-have_metadata )
'Enough of the resource has been obtained that the duration of the resource is
available.
In the case of a video element, the dimensions of the video are also available.
The API will no longer throw an exception when seeking.
No media data is available for the immediate current playback position.'
I think the current point is that the dimensions of the video are available(in
init segment).
No media data is available (We only have header data. Don't have coded frame
data).

[philipj_slow, 2014-11-20 09:11:17]

On 2014/11/20 06:49:19, jiajia.qin wrote:
> On 2014/11/19 10:20:58, philipj wrote:
> > On 2014/11/19 06:30:07, jiajia.qin wrote:
> > > On 2014/11/18 09:24:23, philipj wrote:
> > > > It seems to me like the ReportMetadata() change is independent of the
> > > > PIPELINE_ERROR_DECODE change, is there a dependency between them that
I'm
> > > > missing?
> > > ReportMetadata is only called by here. I can't find other places to use
it.
> > > Here, when demuxer_ has finished parsing the initialization segment,
> > > Pipeline::OnStateTransition will be executed. According
> > >
> >
>
to(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-init-segment-received)
> > > step 6.2, we should call
> > > ReportMetadata so that HTMLMediaElement can set readyState attribute to
> > > HAVE_METADATA. 
> > 
> > OK, so without this change, appending only an initialization segment would
not
> > cause the HTMLMediaElement to reach HAVE_METADATA? That sounds like a
separate
> > bug, if so can you separate it into a separate CL with its own test?
> > 
> Without this change, appending only an initialization segment would also cause
> HTMLMediaElement.readyState to reach HAVE_METADATA.
> But it will happen in a delay(it will have to wait until renderer_ is
> initialized.
> But during this, parser maybe have parsed some media data).
> I do this change is to reduce this delay.

OK, doing it sooner seems better. The difference is observable by JavaScript,
though, isn't it? If you append data and do setTimeout(0), I'm guessing
previously it wouldn't have reached HAVE_METADATA but now it's guaranteed to? I
don't think a test doing that is necessary though.

>
https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
> > > > media/base/pipeline.cc:304: ReportMetadata();
> > > > Will this affect both MSE and normal playback? The ReportMetadata() for
> the
> > > > kPlaying case looks odd and this more sane, but will this affect when
e.g.
> > the
> > > > loadedmetadata event is fired? Do all LayoutTests still pass?
> > > I think it won't affect loadedmetadata event. In my side, I haven't
noticed
> > any
> > > regression.
> > 
> > That seems odd, if I understand kPlaying correctly it's when everything is
> > prerolled and ready to play, even if not actually playing. That should be
much
> > later than simply successfully initializing the demuxer. Is the init segment
> > enough to set up the decoders as well? I'm assuming that's needed to know
the
> > video size for certain? If so, is what pipeline.h documents as 'When all
> filter
> > initialization states have completed, we are implicitly in a "Paused"
state.'
> > the time to report metadata?
> I think the init segment is enough to set up the decoders (When parsing the
init
> segment,
> we can get all info to config the DemuxerStream. Then, it can be used to
> initialize the renderer_
> which will take charge to initialize the decoder). 
> 
> I agree with you about kPlaying's meaning (when pipeline state is kPlaying,
> it means all init/config work is done including demuxer, renderer, decoder.
> But it hasn't begin to decode any data).
> I think it doesn't conflict. I checked HAVE_METADATA's definition.
> ( http://www.w3.org/TR/html5/embedded-content-0.html#dom-media-have_metadata )

(When implementing or writing tests, never look at a spec in /TR/ if there's a
more recent one. For HTML, https://html.spec.whatwg.org/multipage/ is the most
upstream version.)

> 'Enough of the resource has been obtained that the duration of the resource is
> available.
> In the case of a video element, the dimensions of the video are also
available.
> The API will no longer throw an exception when seeking.
> No media data is available for the immediate current playback position.'
> I think the current point is that the dimensions of the video are available(in
> init segment).
> No media data is available (We only have header data. Don't have coded frame
> data).

The question is, is the container metadata found by the demuxer used to report
the video size, or is the size from the video decoder used? I'm thinking of the
case where they disagree, which I'm assuming is not a fatal error in some
formats.

[philipj_slow, 2014-11-20 09:11:45]

Also, the MSE spec has moved, new link should be
http://w3c.github.io/media-source/#byte-stream-formats

[jiajia.qin, 2014-11-20 10:57:17]

On 2014/11/20 09:11:17, philipj wrote:
> On 2014/11/20 06:49:19, jiajia.qin wrote:
> > On 2014/11/19 10:20:58, philipj wrote:
> > > On 2014/11/19 06:30:07, jiajia.qin wrote:
> > > > On 2014/11/18 09:24:23, philipj wrote:
> > > > > It seems to me like the ReportMetadata() change is independent of the
> > > > > PIPELINE_ERROR_DECODE change, is there a dependency between them that
> I'm
> > > > > missing?
> > > > ReportMetadata is only called by here. I can't find other places to use
> it.
> > > > Here, when demuxer_ has finished parsing the initialization segment,
> > > > Pipeline::OnStateTransition will be executed. According
> > > >
> > >
> >
>
to(https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-init-segment-received)
> > > > step 6.2, we should call
> > > > ReportMetadata so that HTMLMediaElement can set readyState attribute to
> > > > HAVE_METADATA. 
> > > 
> > > OK, so without this change, appending only an initialization segment would
> not
> > > cause the HTMLMediaElement to reach HAVE_METADATA? That sounds like a
> separate
> > > bug, if so can you separate it into a separate CL with its own test?
> > > 
> > Without this change, appending only an initialization segment would also
cause
> > HTMLMediaElement.readyState to reach HAVE_METADATA.
> > But it will happen in a delay(it will have to wait until renderer_ is
> > initialized.
> > But during this, parser maybe have parsed some media data).
> > I do this change is to reduce this delay.
> 
> OK, doing it sooner seems better. The difference is observable by JavaScript,
> though, isn't it? If you append data and do setTimeout(0), I'm guessing
> previously it wouldn't have reached HAVE_METADATA but now it's guaranteed to?
I
> don't think a test doing that is necessary though.
> 
I think so.

> >
>
https://codereview.chromium.org/710693003/diff/1/media/base/pipeline.cc#newco...
> > > > > media/base/pipeline.cc:304: ReportMetadata();
> > > > > Will this affect both MSE and normal playback? The ReportMetadata()
for
> > the
> > > > > kPlaying case looks odd and this more sane, but will this affect when
> e.g.
> > > the
> > > > > loadedmetadata event is fired? Do all LayoutTests still pass?
> > > > I think it won't affect loadedmetadata event. In my side, I haven't
> noticed
> > > any
> > > > regression.
> > > 
> > > That seems odd, if I understand kPlaying correctly it's when everything is
> > > prerolled and ready to play, even if not actually playing. That should be
> much
> > > later than simply successfully initializing the demuxer. Is the init
segment
> > > enough to set up the decoders as well? I'm assuming that's needed to know
> the
> > > video size for certain? If so, is what pipeline.h documents as 'When all
> > filter
> > > initialization states have completed, we are implicitly in a "Paused"
> state.'
> > > the time to report metadata?
> > I think the init segment is enough to set up the decoders (When parsing the
> init
> > segment,
> > we can get all info to config the DemuxerStream. Then, it can be used to
> > initialize the renderer_
> > which will take charge to initialize the decoder). 
> > 
> > I agree with you about kPlaying's meaning (when pipeline state is kPlaying,
> > it means all init/config work is done including demuxer, renderer, decoder.
> > But it hasn't begin to decode any data).
> > I think it doesn't conflict. I checked HAVE_METADATA's definition.
> > ( http://www.w3.org/TR/html5/embedded-content-0.html#dom-media-have_metadata
)
> 
> (When implementing or writing tests, never look at a spec in /TR/ if there's a
> more recent one. For HTML, https://html.spec.whatwg.org/multipage/ is the most
> upstream version.)
> 
Thanks for the warning.

> > 'Enough of the resource has been obtained that the duration of the resource
is
> > available.
> > In the case of a video element, the dimensions of the video are also
> available.
> > The API will no longer throw an exception when seeking.
> > No media data is available for the immediate current playback position.'
> > I think the current point is that the dimensions of the video are
available(in
> > init segment).
> > No media data is available (We only have header data. Don't have coded frame
> > data).
> 
> The question is, is the container metadata found by the demuxer used to report
> the video size, or is the size from the video decoder used? I'm thinking of
the
> case where they disagree, which I'm assuming is not a fatal error in some
> formats.

I think it's the former 'the container metadata found by the demuxer used to
report the video size'.
My understanding is Video decoder's config should come from the container
metadta.

[philipj_slow, 2014-11-20 12:50:06]

On 2014/11/20 10:57:17, jiajia.qin wrote:
> On 2014/11/20 09:11:17, philipj wrote:
> > The question is, is the container metadata found by the demuxer used to
report
> > the video size, or is the size from the video decoder used? I'm thinking of
> the
> > case where they disagree, which I'm assuming is not a fatal error in some
> > formats.
> 
> I think it's the former 'the container metadata found by the demuxer used to
> report the video size'.
> My understanding is Video decoder's config should come from the container
> metadta.

From http://www.webmproject.org/docs/vp8-sdk/example__simple__decoder.html it
looks like libvpx is able to determine the width and height without external
information.

I'll leave this question for the actual OWNERS, though, I have no further
questions or objects on this CL.

[DaleCurtis, 2014-11-20 18:59:51]

https://codereview.chromium.org/710693003/diff/20001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/710693003/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:299: // When demuxer has finished the init segment, it
should call ReportMetadata
This is too general of a location for this code; this method simply trampolines
to the state machinery.

It should instead be immediately prior to the next state transition which should
be kInitRenderer.

[jiajia.qin, 2014-11-21 01:59:47]

On 2014/11/20 18:59:51, DaleCurtis wrote:
> https://codereview.chromium.org/710693003/diff/20001/media/base/pipeline.cc
> File media/base/pipeline.cc (right):
> 
>
https://codereview.chromium.org/710693003/diff/20001/media/base/pipeline.cc#n...
> media/base/pipeline.cc:299: // When demuxer has finished the init segment, it
> should call ReportMetadata
> This is too general of a location for this code; this method simply
trampolines
> to the state machinery.
> 
> It should instead be immediately prior to the next state transition which
should
> be kInitRenderer.

Oh, I see what you are saying. Sorry for my misunderstanding that I though
you mean why not after initial renderer. Have done it. Thanks.

[DaleCurtis, 2014-11-21 18:55:53]

dalecurtis@chromium.org changed required reviewers:
+ wolenetz@chromium.org

[DaleCurtis, 2014-11-21 18:55:54]

lgtm, but please let wolenetz@ give final approval.

[wolenetz, 2014-11-21 19:00:49]

I'll take a look at this and a few others today. My apologies for delay, and
thank you, philipj@ and dalecurtis@ for comments so far.

[wolenetz, 2014-11-22 00:07:53]

On 2014/11/21 19:00:49, wolenetz wrote:
> I'll take a look at this and a few others today. My apologies for delay, and
> thank you, philipj@ and dalecurtis@ for comments so far.

My CR of this and others will likely slip until Monday; my apologies for the
additional delay.

[wolenetz, 2014-11-26 23:35:49]

On 2014/11/22 00:07:53, wolenetz wrote:
> On 2014/11/21 19:00:49, wolenetz wrote:
> > I'll take a look at this and a few others today. My apologies for delay, and
> > thank you, philipj@ and dalecurtis@ for comments so far.
> 
> My CR of this and others will likely slip until Monday; my apologies for the
> additional delay.

My sincere apologies - I want to due full diligence on this CR, and have not yet
found time.
Initial comment: Is there a layout test that shows how the old behavior fails? I
see https://codereview.chromium.org/742653002, but since this chromium side
hasn't yet landed, I would expect either a candidate CL containing a test that
would fail old behavior and pass new behavior, along with a temporary
TestExpectation for the layout test to fail until the new behavior is landed. I
don't see any failing related layout test. Am I missing something?

[jiajia.qin, 2014-11-27 06:07:57]

On 2014/11/26 23:35:49, wolenetz wrote:
> On 2014/11/22 00:07:53, wolenetz wrote:
> > On 2014/11/21 19:00:49, wolenetz wrote:
> > > I'll take a look at this and a few others today. My apologies for delay,
and
> > > thank you, philipj@ and dalecurtis@ for comments so far.
> > 
> > My CR of this and others will likely slip until Monday; my apologies for the
> > additional delay.
> 
> My sincere apologies - I want to due full diligence on this CR, and have not
yet
> found time.
> Initial comment: Is there a layout test that shows how the old behavior fails?
I
> see https://codereview.chromium.org/742653002, but since this chromium side
> hasn't yet landed, I would expect either a candidate CL containing a test that
> would fail old behavior and pass new behavior, along with a temporary
> TestExpectation for the layout test to fail until the new behavior is landed.
I
> don't see any failing related layout test. Am I missing something?

https://codereview.chromium.org/742653002 will fail old behavior and pass new
behavior.
Patch set 2 works correctly with that LayoutTest. Patch set 3 fails because that
I move
ReportMetadata() to case kInitRenderer. It seems that task_runner_->PostTask
will have a delay.
So putting ReportMetadata() in the beginning of OnStateTransition is the most
suitable position. 
Patch set 5 has corrected the behavior. If the patch is landed,
https://codereview.chromium.org/742653002
should pass.

[jiajia.qin, 2014-12-02 01:47:51]

Hi, wolenetz. Any comments?

[jiajia.qin, 2014-12-08 01:47:48]

Hi, wolenetz. Sorry to push you again. I just want to close this Cl as soon as
possible since it is a very simple change. Any comments for this?

[wolenetz, 2015-01-08 22:36:47]

On 2014/12/08 01:47:48, jiajia.qin wrote:
> Hi, wolenetz. Sorry to push you again. I just want to close this Cl as soon as
> possible since it is a very simple change. Any comments for this?

My apologies. I'll get to this CR shortly.

Thanks,
Matt

[DaleCurtis, 2015-01-15 23:14:29]

Turns out this is also breaking preload=metadata! I don't think PS#5 is correct,
PS#3 which does the metadata report during kInitRenderer is correct.

Moving it to before the state transition task doesn't fix any "delay" in
reporting, it just makes it work. For your use case here it seems there's still
a race condition between when the renderer is initialized and a decode error
might be fired. Which seems weird... Can you elaborate on what broke when you
moved things under kInitRenderer?

[jiajia.qin, 2015-01-16 06:11:21]

On 2015/01/15 23:14:29, DaleCurtis wrote:
> Turns out this is also breaking preload=metadata! I don't think PS#5 is
correct,
> PS#3 which does the metadata report during kInitRenderer is correct.
> 
> Moving it to before the state transition task doesn't fix any "delay" in
> reporting, it just makes it work. For your use case here it seems there's
still
> a race condition between when the renderer is initialized and a decode error
> might be fired. Which seems weird... Can you elaborate on what broke when you
> moved things under kInitRenderer?

DaleCurtis, PS#3 seems that the result is race condition. Sometimes, it can pass
my test case.
Sometimes, it can't. When fails, the mediaElement.readyState is HAVE_NOTHING
which expected HAVE_METADATA.
I think the reason is when ChunkDemuxer finish initialization and report to
pipeline. ChunkDemuxer still
continue parser the left data. It won't wait pipeline to transfer to
kInitRenderer. This two threads are
concurrent. So the result is uncertain.

[DaleCurtis, 2015-01-16 21:16:02]

I think the real issue with this code is that ChunkDemuxer::Initialize()
post-tasks the init_cb to the media thread which is then post-task'd again to
the same thread since StateTransitionTask always posts.

This means that even though the init_cb "runs" first on the media thread, it
then posts once more to actually complete initialization. During this time the
ChunkDemuxer has issued a host_->OnDemuxerError which already had another task
posted; thus they get interleaved incorrectly.

The simplest solution might be to just not BTCL the init_cb and remove the
task_runner check for OnStateTransition, but I'm still thinking about it.

[DaleCurtis, 2015-01-16 22:16:11]

I've put together https://codereview.chromium.org/827013005/ which should
resolve that issue, but it needs some discussion.

[DaleCurtis, 2015-01-21 21:44:30]

Hi, I've landed my CL, can you try moving the code back to PS#3 and see if this
passes your tests?

[jiajia.qin, 2015-01-22 05:55:06]

On 2015/01/21 21:44:30, DaleCurtis wrote:
> Hi, I've landed my CL, can you try moving the code back to PS#3 and see if
this
> passes your tests?

Done. The test case passed. Thanks.

[DaleCurtis, 2015-01-23 23:56:01]

Thanks, this should be ready to land then. lgtm

[wolenetz, 2015-01-24 01:13:41]

This is looking good to me % nit, and:
would it be possible to add some pipeline integration test(s) that cover this
behavior change?

https://codereview.chromium.org/710693003/diff/100001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/710693003/diff/100001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1351: case INITIALIZED: {
nit: is enclosure of case in {} necessary? It seems inconsistent w.r.t. rest of
this switch.

[jiajia.qin, 2015-01-26 01:49:30]

On 2015/01/24 01:13:41, wolenetz wrote:
> This is looking good to me % nit, and:
> would it be possible to add some pipeline integration test(s) that cover this
> behavior change?

I think the layout test (https://codereview.chromium.org/742653002) has covered
it.

>
https://codereview.chromium.org/710693003/diff/100001/media/filters/chunk_dem...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/710693003/diff/100001/media/filters/chunk_dem...
> media/filters/chunk_demuxer.cc:1351: case INITIALIZED: {
> nit: is enclosure of case in {} necessary? It seems inconsistent w.r.t. rest
of
> this switch.

Done

[jiajia.qin, 2015-01-26 01:50:04]

The CQ bit was checked by jiajia.qin@intel.com

[commit-bot: I haz the power, 2015-01-26 01:50:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710693003/120001

[commit-bot: I haz the power, 2015-01-26 01:50:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-26 01:50:23]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[wolenetz, 2015-01-26 21:10:51]

On 2015/01/26 01:49:30, jiajia.qin wrote:
> On 2015/01/24 01:13:41, wolenetz wrote:
> > This is looking good to me % nit, and:
> > would it be possible to add some pipeline integration test(s) that cover
this
> > behavior change?
> 
> I think the layout test (https://codereview.chromium.org/742653002) has
covered
> it.
> 
> >
>
https://codereview.chromium.org/710693003/diff/100001/media/filters/chunk_dem...
> > File media/filters/chunk_demuxer.cc (right):
> > 
> >
>
https://codereview.chromium.org/710693003/diff/100001/media/filters/chunk_dem...
> > media/filters/chunk_demuxer.cc:1351: case INITIALIZED: {
> > nit: is enclosure of case in {} necessary? It seems inconsistent w.r.t. rest
> of
> > this switch.
> 
> Done

LGTM. I missed seeing the layout test referencing in my CR last week of this.

[wolenetz, 2015-01-26 21:11:02]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2015-01-26 21:11:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710693003/120001

[commit-bot: I haz the power, 2015-01-26 22:14:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-26 22:14:29]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jiajia.qin, 2015-01-27 09:02:01]

wolenetz, I modified the corresponding unit tests. Please review
pipeline_unittest.cc and chunk_demuxer_unittest.cc. Sorry that I only ran layout
test and missed media_unittests.

[philipj_slow, 2015-01-27 09:39:09]

Jiajia, I've nominated you for try bot access. If granted, you will be able to
start try bots before CQ to find this kind of problem and avoid re-review.

[jiajia.qin, 2015-01-27 09:51:51]

On 2015/01/27 09:39:09, philipj_UTC7 wrote:
> Jiajia, I've nominated you for try bot access. If granted, you will be able to
> start try bots before CQ to find this kind of problem and avoid re-review.

Great. Big thanks, Philipj.

[DaleCurtis, 2015-01-27 19:51:29]

Still lgtm.

[wolenetz, 2015-01-27 19:53:05]

I've +1'ed the request for your try bot access (not sure +1 is even necessary,
but can't hurt :)).

LGTM

[DaleCurtis, 2015-01-27 20:28:25]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-01-27 20:29:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710693003/140001

[commit-bot: I haz the power, 2015-01-27 21:23:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-01-27 21:26:47]

Patchset 8 (id:??) landed as
https://crrev.com/5025fd8f7f15225b454ad37bdec8e696db4330f3
Cr-Commit-Position: refs/heads/master@{#313358}