Bug 139075 – [MSE] Unset timestamps of trackbuffers during Reset Parser State algorithm

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

[Bartlomiej Gajda]

Specification in here: http://www.w3.org/TR/2014/CR-media-source-20140717/#sourcebuffer-reset-parser-state
requires from us to unset timestamps for track buffers on abort().

[Bartlomiej Gajda]

Created attachment 242251 [details]
proposed patch

perhaps m_private->abort() should be renamed to m_private->resetParserState() ?

[Sam Weinig]

Seems like this needs a test case.

[Jer Noble]

(In reply to comment #2)
> Seems like this needs a test case.

Yep. You could write a test case using the Mock media source that pushes in a random-access sample, then resets the parser state and pushes in non-random-access samples and verifies that those samples were discarded and not enqueued.

[Sam Weinig]

Comment on attachment 242251 [details]
proposed patch

r- for tests.

[Bartlomiej Gajda]

Created attachment 242312 [details]
Patch with LayoutTest as suggested by Jer Noble

Added sample layout test which adds 4 samples, adds and aborts fifth append, and then tries to add 5 more non sync samples. After all of that checks for dropped samples, and duration. 
Shall be enough to test this?

[Jer Noble]

Comment on attachment 242312 [details]
Patch with LayoutTest as suggested by Jer Noble

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

r=me, with nits in the new layout test. Please upload a new version, and I'll cq+ it.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:34
> +        nextRequest = 0;
> +

No need to re-initialize this variable here.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:40
> +    function finishTest() {

It would be better if this function came after `sourceUpdated()`.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:44
> +        /*
> +            Only first sample was sync, after abort there were only non sync samples, so they should be dropped,
> +            as trackBuffer will have needsRandomAccessPoint set.
> +        */

This comment should go in the ChangeLog, instead of in the test.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:47
> +        var droppedAmount = sampleCount - abortAfter - 1; // 1 sample will be canceled

Since this variable is just subtracting constant values from one another, you can just define this up at the top of the file with the other const values.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:61
> +        if (nextRequest < sampleCount) {
> +            sourceBuffer.appendBuffer(
> +                makeASample(nextRequest, nextRequest, 1, 1,
> +                (nextRequest == 0) ? SAMPLE_FLAG.SYNC : SAMPLE_FLAG.NONE
> +                ));
> +        }
> +        else {
> +            finishTest();
> +        }

WebKit style has you invert the "if()" logic, call finishTest(), and return early.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:65
> +        if (nextRequest == abortAfter) {
> +            sourceBuffer.abort(); // need to do this after appendBuffer, before `updateend`
> +        }

WebKit style also has you remove the braces from single-line if clauses. I don't think the comment is necessary.

> LayoutTests/media/media-source/media-source-append-nonsync-sample-after-abort.html:72
> +    <video controls width=960 height=510></video>

No need to specify a width and height here.

[Bartlomiej Gajda]

Created attachment 242317 [details]
Patch with LayoutTest , fixed style

> No need to re-initialize this variable here.

> It would be better if this function came after `sourceUpdated()`.

> This comment should go in the ChangeLog, instead of in the test.

> Since this variable is just subtracting constant values from one another,
> you can just define this up at the top of the file with the other const
> values.

> WebKit style has you invert the "if()" logic, call finishTest(), and return
> early.

> WebKit style also has you remove the braces from single-line if clauses. I
> don't think the comment is necessary.

> No need to specify a width and height here.

Fixed above, thanks!

[WebKit Commit Bot]

Comment on attachment 242317 [details]
Patch with LayoutTest , fixed style

Clearing flags on attachment: 242317

Committed r176594: <http://trac.webkit.org/changeset/176594>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.