1401147 - video shouldn't be playing when the media source doesn't have any source buffer

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[Alastor Wu [:alwu]]

This test page does the following things,

STR.
1. add the source buffer 
2. append data 
3. receiving updateend (append success)
4. remove the present source buffer
5. play video

Expect.
6. video can't start playing

Actual.
6. video can still be playing

We need to implement this todo[1] and make thing works.

[1] http://searchfox.org/mozilla-central/source/dom/media/mediasource/MediaSource.cpp#338

Comment 1

[Alastor Wu [:alwu]]

Attachment: Log with MediaSource:5

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1401147 - Empty track buffer content upon detach.

Review commit: https://reviewboard.mozilla.org/r/181252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181252/

Comment 3

[Alastor Wu [:alwu]]

After applying your patch, I still see some problems.
These behaviors seem wrong.

1. The video still shows the first frame
2. The duration of media source still have a value (60.63)

In addition, could you please add a test?
Thanks!

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #3)
> After applying your patch, I still see some problems.
> These behaviors seem wrong.
> 
> 1. The video still shows the first frame

There's no reason it shouldn't. After all, readyState can have reached HAVE_CURRENT_FRAME. 
No where does it state in the spec that the canvas is to be cleared.
Compare this to removing network access while playing.
Playback will stop, but whatever is currently being displayed is still stopped.

> 2. The duration of media source still have a value (60.63)
same as above.
It's fundamentally the same as pulling the network cable, or manually emptying the sourcebuffer with remove. duration shouldn't change.

https://w3c.github.io/media-source/#dom-mediasource-removesourcebuffer

There's nothing about changing the duration, or moving the readyState to HAVE_NOTHING (which would be required if you reset the duration (part of the metadata).

> 
> In addition, could you please add a test?
> Thanks!

Comment 5

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> > 2. The duration of media source still have a value (60.63)
> same as above.
> It's fundamentally the same as pulling the network cable, or manually
> emptying the sourcebuffer with remove. duration shouldn't change.
> 
> https://w3c.github.io/media-source/#dom-mediasource-removesourcebuffer
> 
> There's nothing about changing the duration, or moving the readyState to
> HAVE_NOTHING (which would be required if you reset the duration (part of the
> metadata).
> 

Make sense.

> (In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #3)
> There's no reason it shouldn't. After all, readyState can have reached
> HAVE_CURRENT_FRAME. 
> No where does it state in the spec that the canvas is to be cleared.
> Compare this to removing network access while playing.
> Playback will stop, but whatever is currently being displayed is still
> stopped.

Step11 said that "Destroy all resources for sourceBuffer" [1].
If we destroyed all its resources, should the video frame also be removed? 
If the video frame was destroyed, why we could show it for user?

If this behavior is not well-defined on the spec, should we file a bug?

[1] https://w3c.github.io/media-source/#dom-mediasource-removesourcebuffer

---

In addition, I wanted to run my tests on Chrome to see the behavior, but all of them [1] can't run successfully.
Is my usage way incorrect? or that is Chrome's problem?

Thanks!

[1] https://alastor0325.github.io/htmltests/    (mse_XXX.html)

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8909752 [details]
Bug 1401147 - Empty track buffer content upon detach.

https://reviewboard.mozilla.org/r/181252/#review187010

r+ with adding test.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #5)
> Step11 said that "Destroy all resources for sourceBuffer" [1].
> If we destroyed all its resources, should the video frame also be removed? 
> If the video frame was destroyed, why we could show it for user?
> 

the video frame or canvas isn't part of the sourcebuffer. the sourcebuffer contains undemuxed / undecoded frames. It really for all purpose is to be considered like a network resource.


> If this behavior is not well-defined on the spec, should we file a bug?
> 
> [1] https://w3c.github.io/media-source/#dom-mediasource-removesourcebuffer

In the spirit of the spec, there's nothing related to what you suggest.

If you were detaching the MediaSource itself then yes:
https://w3c.github.io/media-source/#mediasource-detach

But here we're just removing a source buffer.
This isn't a final situation, you can perfectly add a new sourcebuffer if desired. In which case you want playback to resume.

For all intent and purpose, removing the source buffer to me is akin to calling SourceBuffer::remove(0, +oo) on it. You've emptied the source buffer, but you haven't destroyed the media source.

Feel free to open a new bug on the MSE spec however if you feel it should be clarified.
the more clarification the better :)

> ---
> 
> In addition, I wanted to run my tests on Chrome to see the behavior, but all
> of them [1] can't run successfully.
> Is my usage way incorrect? or that is Chrome's problem?

the error returned is:
Uncaught DOMException: Failed to execute 'removeSourceBuffer' on 'MediaSource': The SourceBuffer provided is not contained in this MediaSource.
    at removeBuffer (https://alastor0325.github.io/htmltests/mse_remove_sourcebuffer.html:39:23)
    at SourceBuffer.sourceBuffer.addEventListener.once (https://alastor0325.github.io/htmltests/mse_remove_sourcebuffer.html:41:9)

the code seems correct to me, chrome error isn't.

when you don't call removeBuffer() does it works (that is show a video frame)?

Feel free to take this bug and write a test for it...

Comment 8

[Jean-Yves Avenard [:jya]]

Seeing that all browsers appear to behave differently when this situation above, maybe it is worth better defining what needs to be done.

Comment 9

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> the video frame or canvas isn't part of the sourcebuffer. the sourcebuffer
> contains undemuxed / undecoded frames. It really for all purpose is to be
> considered like a network resource.

Hmm, seem that the decoded frame is not in the field of this API, it's more related with MediaElement itself.

> 
> In the spirit of the spec, there's nothing related to what you suggest.
> 
> If you were detaching the MediaSource itself then yes:
> https://w3c.github.io/media-source/#mediasource-detach
> 
> But here we're just removing a source buffer.
> This isn't a final situation, you can perfectly add a new sourcebuffer if
> desired. In which case you want playback to resume.
> 
> For all intent and purpose, removing the source buffer to me is akin to
> calling SourceBuffer::remove(0, +oo) on it. You've emptied the source
> buffer, but you haven't destroyed the media source.
> 
> Feel free to open a new bug on the MSE spec however if you feel it should be
> clarified.
> the more clarification the better :)

OK, I'll file a spec bug later.

> the error returned is:
> Uncaught DOMException: Failed to execute 'removeSourceBuffer' on
> 'MediaSource': The SourceBuffer provided is not contained in this
> MediaSource.
>     at removeBuffer
> (https://alastor0325.github.io/htmltests/mse_remove_sourcebuffer.html:39:23)
>     at SourceBuffer.sourceBuffer.addEventListener.once
> (https://alastor0325.github.io/htmltests/mse_remove_sourcebuffer.html:41:9)
> 
> the code seems correct to me, chrome error isn't.
> 
> when you don't call removeBuffer() does it works (that is show a video
> frame)?

Yes, I have other tests, I think they're more common usage way. (mse_higher_explicit_duration.html/mse_no_end_of_stream.html)

However, none of them could be run on Chrome. So I'm confused whether my implementation is wrong or other browsers are wrong...

And it makes me surprise, I think the usage of MSE should be more compatible.

> Feel free to take this bug and write a test for it...

Sure, I'll write a test and ask for a review.

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1401147 - part1 : empty track buffer content upon detach.

After detached the source buffer, we would reset all its resource and shutdown
demuxer.

Review commit: https://reviewboard.mozilla.org/r/182102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182102/

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1401147 - part2 : add test.

Review commit: https://reviewboard.mozilla.org/r/182104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182104/

Comment 12

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c98771be64358e7d8f7b8b0bd506a0072259081

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

https://reviewboard.mozilla.org/r/182104/#review187464

::: dom/media/mediasource/test/test_PlayWithoutValidSourceBuffer.html:39
(Diff revision 1)
> +  el.play();
> +  setTimeout(function() {
> +    // The ready state of media element is HAVE_FUTURE_DATA, it means we have
> +    // a few frames. Therefore, the current time might not be zero but it would
> +    // be very close to it.
> +    ok(Math.abs(el.currentTime - 0.0) < 0.001,

It's either 0 or it's not.

In either case your comment isn't correct.

if it has a few frames buffered (as they have already been decoded before the source buffer got removed) testing that it's within 0.001 s is likely going to cause intermittent.

el.currentTime - 0.0 makes no sense either.

Comment 14

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #13)
> It's either 0 or it's not.
> 
> In either case your comment isn't correct.
> 
> if it has a few frames buffered (as they have already been decoded before
> the source buffer got removed) testing that it's within 0.001 s is likely
> going to cause intermittent.

Do you mean we should not test with current time?
What I saw is the current time would like around ~0.000040.

Any other good ways can we check whether the video is playing?
Thanks!

Comment 15

[Alastor Wu [:alwu]]

Hmm, is it a good idea if I increase the threshold of checking current time? or we need to use another way?
eg. if el.currentTime < 0.1, then the test pass

Comment 16

[Alastor Wu [:alwu]]

I've seen the different current time on my try result, and it indeed causes the fail.

Comment 17

[Alastor Wu [:alwu]]

Comment on attachment 8910667 [details]
Bug 1401147 - part1 : empty track buffer content upon detach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182102/diff/1-2/

Comment 18

[Alastor Wu [:alwu]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182104/diff/1-2/

Comment 19

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4323ac4eaccbae652133d5340dd92c7002e0c2db

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

https://reviewboard.mozilla.org/r/182104/#review187656

::: dom/media/mediasource/test/test_PlayWithoutValidSourceBuffer.html:37
(Diff revision 2)
> +    "buffered range should be 0 when we don't have any source buffer");
> +
> +  info("- play video without the valid source buffer -");
> +  el.play();
> +  await once(el, "waiting");
> +  ok(true, "video should not have enough data to playback continuously.");

this test doesnt add much value, even if you had added data you will hace got a waiting event once currentTime reaches the end as endOfStream() wasnt called.

that buffered attribute is empty should be enough IMHO.

alternatively, you could check that seeking to 0 never completes.

Comment 21

[Alastor Wu [:alwu]]

Comment on attachment 8910667 [details]
Bug 1401147 - part1 : empty track buffer content upon detach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182102/diff/2-3/

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182104/diff/2-3/

Comment 23

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #20)
> that buffered attribute is empty should be enough IMHO.

No, without patch1, video can be playing and its buffered is also empty.
I would add seeking check to ensure the correct behavior (seek would fail cause of lacking enough data).

Comment 24

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a7d2a9e0b2e682508ec641f54d5d4a21ce95801

Comment 25

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #23)
> (In reply to Jean-Yves Avenard [:jya] from comment #20)
> > that buffered attribute is empty should be enough IMHO.
> 
> No, without patch1, video can be playing and its buffered is also empty.
> I would add seeking check to ensure the correct behavior (seek would fail
> cause of lacking enough data).
You're not testing that the video isn't playing. You're testing that the waiting event will be fired.
And even without removing the source buffer, the waiting event *will* be fired. 

You aren't calling EndOfStream, as such ended won't ever be fired. But waiting surely will.

Comment 26

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #25)
> You're not testing that the video isn't playing. You're testing that the
> waiting event will be fired.
> And even without removing the source buffer, the waiting event *will* be
> fired. 
> 
> You aren't calling EndOfStream, as such ended won't ever be fired. But
> waiting surely will.

Have you seen my latest patch? I've changed the test to check whether seeking can be completed as a criteria, instead of checking "waiting" event.

Comment 27

[Jean-Yves Avenard [:jya]]

I was just answering your message (which was an answer to mine), where I was specifically talking about the waiting event.

Comment 28

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #27)
> I was just answering your message (which was an answer to mine), where I was
> specifically talking about the waiting event.

Yes, I just mean, only testing buffered is not enough (not talking about 'waiting', I've known it's not a good criteria from your explanation), so I'm adding your suggestion to seek 0 to ensure the video won't have enough data to playback.

Thanks for those detail reply.

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8910667 [details]
Bug 1401147 - part1 : empty track buffer content upon detach.

https://reviewboard.mozilla.org/r/182102/#review187842

this is a commit of mine which you r+.
If you had kept the commit-id, authorship and review status would be kept

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

https://reviewboard.mozilla.org/r/182104/#review187844

::: dom/media/mediasource/test/test_SeekWithoutValidSourceBuffer.html:26
(Diff revision 3)
> +    ok(true, "start seeking");
> +
> +    let timer = setTimeout(function() {
> +      ok(true, "seeking would never be completed because of lacking enough data.");
> +      resolve();
> +    }, 1000);

should never have timeout or things assuming an operation will complete in a particular amount of time.

This will always end up intermitently failing depending on how busy the machine running the test is.

so let it timeout

Comment 31

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #30)
> should never have timeout or things assuming an operation will complete in a
> particular amount of time.
> 
> This will always end up intermitently failing depending on how busy the
> machine running the test is.
> 
> so let it timeout

Sad, I can't find any ways could make this test stable without using timeout...
So I would remove the playback related checking and only leave buffered status check.

Comment 32

[Alastor Wu [:alwu]]

Comment on attachment 8910667 [details]
Bug 1401147 - part1 : empty track buffer content upon detach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182102/diff/3-4/

Comment 33

[Alastor Wu [:alwu]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182104/diff/3-4/

Comment 34

[Jean-Yves Avenard [:jya]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

https://reviewboard.mozilla.org/r/182104/#review187924

::: dom/media/mediasource/test/test_RemoveSourceBuffer.html:36
(Diff revision 4)
> +  is(el.buffered.length, 0,
> +     "buffered range should be 0 since we don't have any source buffer.");
> +
> +  info("- call endOfStream -");
> +  ms.endOfStream();
> +  is(ms.duration, 0, "duraton should be 0 since we don't have any source buffer.");

may as well also test that activeSourceBuffer is empty and that source buffer list is also empty.

Comment 35

[Alastor Wu [:alwu]]

Comment on attachment 8910668 [details]
Bug 1401147 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182104/diff/4-5/

Comment 36

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31d41715c28706d147474e11066487e6d7799573

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aabb5872e828
part1 : empty track buffer content upon detach. r=jya
https://hg.mozilla.org/integration/autoland/rev/d7435cf7c4e7
part2 : add test. r=jya

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aabb5872e828
https://hg.mozilla.org/mozilla-central/rev/d7435cf7c4e7