1293843 - MDSM should drop future frames past explicit duration

Status: NEW

(Core :: Audio/Video: Playback, defect, P3)

Description

[Jean-Yves Avenard [:jya]]

This causes some intermittent failures in the web-platform-test tests.

Under some circumstances, the MDSM may have already decoded frames that have now been evicted. The MDSM will play those even if the explicit duration got set, which will cause currentTime to go past the set duration and re-adjust the duration once those frames get played.

When the duration is explicitly set with MSE, frames queued past that time should be removed from the queue.

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Jean-Yves Avenard [:jya] from comment #0)
> This causes some intermittent failures in the web-platform-test tests.

Hello JYA,

I've enabled "testing/web-platform/tests/media-source/mediasource-duration.html" and tested for 150 times, but cannot reproduce any unexpected result.
Do you know which tests will fail ?

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Another 100 run on Windows VM, gotcha !

22:24.17 TEST_END: Thread-TestrunnerManager-75 Harness OK. Subtests passed 4/5. Unexpected 1
Test appendBuffer completes previous seek to truncated duration
---------------------------------------------------------------
Expected PASS, got FAIL
assert_equals: Event types match. expected "seeking" but got "timeupdate"

952:18.05 TEST_END: Thread-TestrunnerManager-84 Harness OK. Subtests passed 4/5. Unexpected 1
Test seek starts on duration truncation below currentTime
---------------------------------------------------------
Expected PASS, got FAIL
assert_equals: Event types match. expected "seeking" but got "timeupdate"


It seems a regression of Bug 1148224.
Will dig further to see if these are related to Comment 0

Comment 3

[Jean-Yves Avenard [:jya]]

This is not the error you would typically get. there's various issues in the tests not catering for the timeupdate event.

timeupdate can be fired at any given time. Chrome doesn't and always fire the events in a specific order.

It will be easier for you to reproduce the problem if you bump the video decode ahead pref from the fdefault 2 to something like 30.

So I believe that you can safely ignore the timeupdate intermittent related errors

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

I've tried increase the size of decoded video frames, and repeated about 600 times, but only got another assertion [1].
Expected PASS, got FAIL
  assert_less_than: mediaElement currentTime expected a number less than 0.25 but got 0.271133

But that should not be the issue we are going to fix according to Comment 0. 

For this bug, I supposed that we're looking for assertions here [2]
So I started looking into code, there might be a potential race when the MDSM::mDuration is updated due to the change to MDSM::mExplicitDuration and after that, MDSM::UpdatePlaybackPositionPeriodically() is called right away so that MDSM::mObservedDuration is updated according to the playback position which should be evicted.

I've developed a tentative patch to only remove audio/video data when the MDSM::mDuration is updated according to MDSM::mExplicitDuration.  I'm not sure if that is the right solution because I cannot verify it even reproduce it.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/media-source/mediasource-duration.html#176 
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/media-source/mediasource-duration.html#69-76

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Kilik Kuo [:kikuo] from comment #4)
> I've tried increase the size of decoded video frames, and repeated about 600
> times, but only got another assertion [1].
> Expected PASS, got FAIL
>   assert_less_than: mediaElement currentTime expected a number less than
> 0.25 but got 0.271133
> 
> But that should not be the issue we are going to fix according to Comment 0. 

I think this is exactly one effect of what is described in comment 0

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to Kilik Kuo [:kikuo] from comment #4)
> > I've tried increase the size of decoded video frames, and repeated about 600
> > times, but only got another assertion [1].
> > Expected PASS, got FAIL
> >   assert_less_than: mediaElement currentTime expected a number less than
> > 0.25 but got 0.271133
> > 
> > But that should not be the issue we are going to fix according to Comment 0. 
> 
> I think this is exactly one effect of what is described in comment 0

Look into part of test [1], IIUC, the assertion is triggered because the late of the following to event.
...
test.expectEvent(sourceBuffer, 'updateend', 'sourceBuffer');
test.expectEvent(mediaElement, 'playing', 'Playing triggered');
...

That's why I don't think this is the one, or do I misunderstand the test ?
I'll still try reproducing it again to get the log (sadly, I didn't log it at the first time ...)

[1] https://dxr.mozilla.org/mozilla-central/rev/7e873393cc11d326338779e5a3ed2da031e30936/testing/web-platform/tests/media-source/mediasource-duration.html#154-176

Comment 7

[Jean-Yves Avenard [:jya]]

currentTime being further than expected *is* a result of the failure you describe.
There should be no data past 0.25 as this is what the sourceBuffer.remove() did sourceBuffer.remove(newDuration, Infinity);

with newDuration set to 0.25