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().
Created attachment 242251 [details] proposed patch perhaps m_private->abort() should be renamed to m_private->resetParserState() ?
Seems like this needs a test case.
(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.
Comment on attachment 242251 [details] proposed patch r- for tests.
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?
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.
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!
Comment on attachment 242317 [details] Patch with LayoutTest , fixed style Clearing flags on attachment: 242317 Committed r176594: <http://trac.webkit.org/changeset/176594>
All reviewed patches have been landed. Closing bug.