Bug 139439 – [MSE] Implement Append Error algorithm.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Bartlomiej Gajda]

There is no place currently which executes "3.5.1 Segment Parser Loop" 
paragraph : 
"6.1 If the first initialization segment flag is false, then run the end of stream algorithm with the error parameter set to "decode" and abort this algorithm."

Which means, no 'error' event will be scheduled.

[Bartlomiej Gajda]

Created attachment 242905 [details]
call endofstream on receive sample without first init flag v1

This is the initial patch of this. I'm not sure what should be proper way to fail here as sometimes 'updateend' came before 'error' (from unknown reason) so I rejected that option. I'm open to suggestions.

[Jer Noble]

(In reply to comment #1)
> Created attachment 242905 [details]
> call endofstream on receive sample without first init flag v1

Doesn't this look like a spec bug, that MediaSource.endOfStream() is supposed to call the Append Error Algorithm (on SourceBuffer) without stating explicitly /which/ SourceBuffer is supposed to run the algorithm?  It's a little weird to pass in a SourceBuffer to MediaSource.streamEndedWithError().  After all, what if someone calls "endOfStream('decode')"?

Wouldn't it make more sense for the 3.5.1 - 6.1 step to call the Append Error Algorithm, which in turn calls the Stream Ended Algorithm (rather than vice versa)?

I filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=27548>. Lets see what they say.

> This is the initial patch of this. I'm not sure what should be proper way to
> fail here as sometimes 'updateend' came before 'error' (from unknown reason)
> so I rejected that option. I'm open to suggestions.

There was a bug <https://bugs.webkit.org/show_bug.cgi?id=138394> about events from different GenericEventQueues being fired out of order. Is that change in your tree?

[Bartlomiej Gajda]

> Doesn't this look like a spec bug, that MediaSource.endOfStream() is
> supposed to call the Append Error Algorithm (on SourceBuffer) without
> stating explicitly /which/ SourceBuffer is supposed to run the algorithm? 

> After all, what if someone calls "endOfStream('decode')"?

While it's certainly not as clear as possible, I think this particular scenario - with calling endOfStream('decode') is well guarded by :

"2.2 Methods / endOfStream
If the updating attribute equals true on any SourceBuffer in sourceBuffers, then throw an INVALID_STATE_ERR exception and abort these steps."

So in such case, we won't go into Append Error Algorithm, as we return early. 
And when SourceBuffer is not updating, then we won't go there as well, due to: 

"2.4.7 If error is set to "decode"
   If updating equals true, then run the append error algorithm."

So it seems the only way to actually call it, is by manually calling end of stream algorithm(streamEndedWithError (not the best name anymore)) - which happens only if SourceBuffer calls it - hence this change.
 
I do agree however that this is a litle confusing. 

> It's a little weird to pass in a SourceBuffer to MediaSource.streamEndedWithError().  

For now I could change it to, SourceBuffer calling :

(new) MediaSource::sourceBufferEndOfStreamDecodeError(SB* sb) {
 if (sb->updating()) // or ASSERT actually as it would be weird if called in different context
  sb->appendError()
 streamEndedWithError("decode");
}

So we would discard the default parameter. 
I think we could go with this hardcoded "decode" error design, as there is no "network" eos() inside SB.

This would work for now, as order of event would be the same from outside I guess, since "decode" endOfStream() path start with this "append error" check anyway.
Would that look better to You?

Also other places in SB which do streamEndedWithError(decode) shall use this new approach but in this patch I had not yet figured layout tests for each of those, so I thought I'll add them later.

> Wouldn't it make more sense for the 3.5.1 - 6.1 step to call the Append
> Error Algorithm, which in turn calls the Stream Ended Algorithm (rather than
> vice versa)?

The easiest way would be if 3.5.1 6.1 would call both directly, as in 3.5.6 for example even though there is AppendError there is no `end of stream: decode`.

> There was a bug <https://bugs.webkit.org/show_bug.cgi?id=138394> about
> events from different GenericEventQueues being fired out of order. Is that
> change in your tree?

It should be, but I'll check it out later. Thanks.

[Bartlomiej Gajda]

> This would work for now, as order of event would be the same from outside I
> guess, since "decode" endOfStream() path start with this "append error"
> check anyway.

Scratch that idea - it would revers order of 
sourceended, error, updateend
to
error, updateend, sourceended,

and that would not be as in spec for now.

[Bartlomiej Gajda]

Since bug in MSE spec. was resolved I thought of adding new patch, but I have problem what to do with streamEndedWithError in 

1. sourceBufferPrivateDidReceiveSample():
 around :
"// 1.5.3 If the presentation timestamp or decode timestamp is less than the presentation start"

since this was removed in editor's draft, as per bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27487

2. sourceBufferPrivateDidReceiveRenderingError()
I'm not sure do I understand how this is used.

3. sourceBufferPrivateDidEndStream()
Nobody uses it?

Aside from the places mentioned in change of spec, should I convert 2) & 3) to appendError(true) ? Or leave it as they are?

As for 1) - I could drop it, but there is a bit of other wording issues there (in 3.5.8) which should be resolved, and that should be done probably after Jer Noble's patch : https://bugs.webkit.org/show_bug.cgi?id=139265
so I'll probably leave it for now.

[Bartlomiej Gajda]

Created attachment 243288 [details]
appendError algorithm implemented

Since I've changed it in other places as well I renamed this to 'Implement AppendError', as I think it's more fitting.

On it's own this patch is ok, but as I said, I did not change all of platform specific "streamEndedWithError(decode..." places, as I don't know in whichs cenario they are used. (and should it be with or without eos)

[Jer Noble]

Comment on attachment 243288 [details]
appendError algorithm implemented

View in context: https://bugs.webkit.org/attachment.cgi?id=243288&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1145
> -    // 3.1. Verify the following properties. If any of the checks fail then run the end of stream
> -    // algorithm with the error parameter set to "decode" and abort these steps.
> +    // 3.1. Verify the following properties. If any of the checks fail then run the append error algorithm
> +    // with the decode error parameter set to true and abort these steps.

The documentation changed, but I don't see a call to "appendError(true)" in this patch.  Is this an oversight?

[Bartlomiej Gajda]

> > +    // 3.1. Verify the following properties. If any of the checks fail then run the append error algorithm
> > +    // with the decode error parameter set to true and abort these steps.
> 
> The documentation changed, but I don't see a call to "appendError(true)" in
> this patch.  Is this an oversight?

It is called by SourceBuffer.cpp:941 in this patch:

if (!validateInitializationSegment(segment)) {
 941 appendError(true);

Checks in validation were not changed, only result of fail, which is checked in sourceBufferPrivateDidReceiveInitializationSegment, so I changed previous streamEndedWithError call.

I can add comment pointing to that line if You wish?

[Jer Noble]

(In reply to comment #8)
> > > +    // 3.1. Verify the following properties. If any of the checks fail then run the append error algorithm
> > > +    // with the decode error parameter set to true and abort these steps.
> > 
> > The documentation changed, but I don't see a call to "appendError(true)" in
> > this patch.  Is this an oversight?
> 
> It is called by SourceBuffer.cpp:941 in this patch:
> 
> if (!validateInitializationSegment(segment)) {
>  941 appendError(true);
> 
> Checks in validation were not changed, only result of fail, which is checked
> in sourceBufferPrivateDidReceiveInitializationSegment, so I changed previous
> streamEndedWithError call.
> 
> I can add comment pointing to that line if You wish?

Ah, I see now. It might be worthwhile pulling the comment starting with '3.1. Verify...' up before line 941, so it's clear what's going on. (And marking the comment inside validateInitializationSegment as '(ctd)'.)

[Bartlomiej Gajda]

Created attachment 246129 [details]
appendError algorithm v2

- reordered comments
- added/changed few missing comments
- checked 'Append Error Algorithm', and 'Initialization Segment Received' versus 09 January 2015 Editor Draft version. Added few FIXME's for missing (in that particular version) parts.

[WebKit Commit Bot]

Comment on attachment 246129 [details]
appendError algorithm v2

Clearing flags on attachment: 246129

Committed r179725: <http://trac.webkit.org/changeset/179725>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[zalan]

(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.
This broke 2 tests.
https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK2%20(Tests)/r179736%20(2610)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141320

[Bartlomiej Gajda]

Created attachment 246160 [details]
appendError algorithm v2 + updated layouttests

I 've updated those two tests to correctly expect both 'error' and 'updateend' before 'sourceended' as per spec. (previously there was no 'updateend' before 'sourceended')

[WebKit Commit Bot]

Comment on attachment 246160 [details]
appendError algorithm v2 + updated layouttests

Clearing flags on attachment: 246160

Committed r179762: <http://trac.webkit.org/changeset/179762>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.