abort() shouldn't attempt to interrupt the current buffer append

[jyavenard]

Overview: I believe abort() shouldn't interrupt the Coded Frame Processing Algorithm; as abort() as it's currently described results in non deterministic behavior in regards to which frames may have actually been added to the source buffer.

Rather than interrupt something that is really non-interruptible, it should only guarantee that the next call to appendBuffer or modification to timestampOffset or appendWindow will succeed.

Detailed explanation:

Per spec: "Aborts the current segment and resets the segment parser."

Which then calls the Reset Parser State which defines:

"If the append state equals PARSING_MEDIA_SEGMENT and the input buffer contains some complete coded frames, then run the coded frame processing algorithm until all of these complete coded frames have been processed."

Now, let's look at the behavior on the append state value, which is defined in "Segment Parser Loop". In a typical MSE transaction, it would go from WAITING_FOR_SEGMENT to PARSING_INIT_SEGMENT to (WAITING_FOR_SEGMENT to PARSING_MEDIA_SEGMENT):repeat

Now let's assume an appendBuffer is done with data containing "media_segment1 | media_segment2 | media_segment3" The Segment Parser Loop runs asynchronously and during the time of its execution will update the append state then run the coded frame processing algorithm for a single media segment, then set the append state to WAITING_FOR_SEGMENT and rinse and repeat.

Now we have an abort() and the Reset Parser State step.

Let's assume that at the exact time the Reset Parser State is run, the Segment Parser Loop had been interrupted after just having finished processing media_segment1. As such the append state is WAITING_FOR_SEGMENT.

As the append state is not equal to PARSING_MEDIA_SEGMENT, none of the remaining frames of the input buffer will be processed (as the conditional is AND)

As in step 7 of the Reset Parser State we have "Remove all bytes from the input buffer.", the source buffer following this abort contains only media_segment1.

If however, the abort of the append buffer occurred when the middle of the media_segment1 was being processed, the append state now being PARSING_MEDIA_SEGMENT all remaining complete frames in the input buffer will be processed and the source buffer will now contain all frames of media_segment1, media_segment2 and media_segment3.

The behavior of abort() is as such racy and non deterministic (and that's ignoring the fact that interrupting an asynchronous step is inherently impossible to achieve)

I believe abort() should be made clearer to remove all ambiguities.

abort should now be: 3: If the buffer append or stream append loop algorithms are running then run the following steps:

1. Set the aborting flag to true
2. Wait for the buffer append and stream append loop algorithms to complete. 4: If the updating attribute equals true, then run the following steps: 1.Set the updating attribute to false. 2.Queue a task to fire a simple event named abort at this SourceBuffer object. 3.Queue a task to fire a simple event named updateend at this SourceBuffer object. 5: Run the reset parser state algorithm."

Step 1 of the Reset Parser State should be removed and add a last step (now 8) "8. if the aborting flag equals true then set the aborting flag to false"

Buffer Append Algorithm now becomes:

2. If the segment parser loop algorithm in the previous step was aborted or if the aborting flag is set to true, then abort this algorithm.

Stream Append Loop now becomes:

14. If the segment parser loop algorithm in the previous step was aborted or if the aborting flag is set to true, then abort this algorithm.

Ultimately, the only reason for abort is to guarantee that the next operation relying on the updating attribute value will complete, and that any operations depending on the append state will also succeed (that is changing the mode attribute, and the timeStampOffset attribute)

At least this is how I've seen all DASH players using it (including YouTube)

[wolenetz]

Thanks for filing this, @jyavenard. I agree there is currently some non-determinism that could result due to the timing of abort relative to the segment parser loop and input buffer states. Especially, it is concerning that the spec aborts an asynchronously running algorithm. Cleaning up the latter looks like something for V1.

It's less clear to me that apps don't rely on abort() in fact aborting the processing much earlier than reaching the end of the appended buffer or stream. If an app appends a very large buffer and then "immediately" calls abort(), I think it could reasonably expect that not all of the appended data must be parsed before "abort" and "updateend" are fired (and it can use the buffered attribute to determine what actually happened).

How about this: in addition to the steps in your proposal, add a new first step in the coded frame processing algorithm (which, note, is run at an implementation-defined frequency from the segment parser loop) which aborts if the aborting flag is true; and insert a new step in the segment append loop between the current:

3. If this SourceBuffer is full and cannot accept more media data, then set the buffer full flag to true. New step: If the aborting flag is true, then abort these steps.
4. If the input buffer does not contain a complete media segment, then jump to the need more data step below.

[wolenetz]

In my addition to your proposal, @jyavenard , the segment parser loop would also need a new step after "loop top" which aborts early if the aborting flag is true. If you agree, I'll put together a PR for V1 for this.

I'm marking this V1 for at least fixing the problem of the spec aborting an asynchronously running algorithm without defining precisely what that means.

[jyavenard]

The resulting proposed change would still result in a non deterministic outcome, which I believe we should avoid at all cost. The end result should always be the same regardless of the speed of the agent running. Otherwise, as the MSE implementation is non deterministic, it means that the JS player would still need to have special code to handle frames that may or may not have been added during abort. No assumption can be taken in regards to when the abort actually occurred: It could have had time to process the lot, some or none at all. Even something as simple as: sourceBuffer.appendBuffer(blob); sourceBuffer.abort();

could result in frames being added to the source buffer.

As a result, how the agent decide to handle abort is as good as any, and may as well always make it as if all frames were always processed. I just wanted to remove that confusion.

Unless we change how and when the buffer append algorithm is run (like it must only run at the end of the current loop) so that the above operation always result in no frame being added.

I'm talking from an implementation perspective: an appendBuffer operation is typically very fast, so fast that the time it takes is more often than not totally negligible.

FWIW, Mozilla made gecko abort behave exactly as described above. We use a promise equivalent internally. When appendBuffer is queued, a promise is returned. Upon completion of that promise update/updateend and duration are updated. If abort() is called, the promise is disconnected and we reset the sourceBuffer parser state. Being multi-threaded, the segment parser loop will in fact complete its operation and abort will not block the main thread waiting for the appendBuffer to complete. But those are implementations details.

[wolenetz]

Implementation-wise, Chromium chunks the appended buffer and iterates asynchronously over incrementally parsing them; if an abort occurs and there's still chunks that haven't been dispatched for asynchronous parsing, they're dropped.

For deterministic behavior, an app should await 'updateend'. abort() prior to then, and after appendBuffer(), can indeed produce non-deterministic behavior. This allows for implementations to handle large appends without unduly blocking the synchronous "main thread", while also allowing apps to respond faster to conditions which could require abort() (like responding to user input).

I'm not totally bought into avoiding non-determinism in this case at all costs: this case is somewhat analogous to an app setting src=some regular URL, issuing play(), and then making some decision to interrupt the regular resource fetch algorithm before HAVE_ENOUGH_DATA: it's non-deterministic how much data might have been fetched and buffered before the app interrupts the resource fetch algorithm.

[jyavenard]

gecko implementation of the buffer append algorithm is entirely run in a secondary thread (our whole MSE and media playback stack is now heavily multi-threaded since version 42), it will never block the main thread, regardless of the length of the buffer appended. It may take a while for updateend to be fired (particularly if verbose logging is used), but blocking it will not. If non deterministic is something wanted, we may as well close this report then.

[jyavenard]

Oh, terribly sorry, I didn't follow properly what you wrote (nasty cold today)... I see what you mean about blocking, that if we have abort() waiting for buffer append algorithm to complete. And that could be blocking which you want to avoid.

You're entirely right. Though I don't think any implementations would actually wait. Our sourcebuffer operations are stateful, we don't really have to wait as such...

So your proposal of narrowing "where" the buffer append algorithm / segment parser loop will be aborted at least narrow the effects.

[jdsmith3000]

@jyavenard Do you consider this an interop issue? And is the mitigation proposed by @Wolenetz sufficient?

[paulbrucecotton]

@jyavenard - We URGENTLY need to know your opinion whether the mitigation proposed above is adequate. We have marked this issue with the V1 Milestone and we are trying to complete V1 issues by June 9. See MSE timeline).

@jyavenard wrote:

So your proposal of narrowing "where" the buffer append algorithm / segment parser loop will be aborted at least narrow the effects.

Can you confirm that you support resolving this issue with ONLY this change?

[jyavenard]

Upon further thinking, I think only my first suggestion would be adequate as I feel that non deterministic outcome is bad, and that attempting to interrupt an asynchronous task is even worse.

Upon re-reading my original suggestion, and as abort() will wait for Buffer Append Algorithm and stream Append Loop Algorithms to complete, there is no need to modify those two. Nor worry about an abort flag.

So you may close this as WONTFIX if you disagree with my suggestions. Regardless of the outcome regarding the resolution, seeing that the result of abort is non deterministic, I feel that the UA is free to implement it any way it wishes. And as such, may as well process all samples contained the input buffer at all time.

[wolenetz]

For V1, since no change is needed, we're closing this issue as WONTFIX, per editors sync today.

@jyavenard I think we're on the same page, though it seems a reach (but in scope of current language) for abort()'s step "Abort the buffer append and stream append loop algorithms if they are running" to let those algorithms run fully to completion before aborting them. This is a quality of implementation variance that is compliant with spec's language.