Closed Bug 1401147 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Depends on 1 open bug,
URL
)

Details

Attachments

(3 files, 1 obsolete file)

Log with MediaSource:5
67.87 KB, text/plain
Details
Bug 1401147 - part1 : empty track buffer content upon detach.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1401147 - part2 : add test.
59 bytes, text/x-review-board-request
jya
: review+
Details 
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
Attached file Log with MediaSource:5 — 

    
    

    
      
  


  

    
  
Assignee: alwu → jyavenard

    
    

    
  
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!
Flags: needinfo?(jyavenard)

    
    

    
  
(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!
Flags: needinfo?(jyavenard)

    
    

    
  
(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)
Flags: needinfo?(jyavenard)

    
    

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

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

r+ with adding test.

        Attachment #8909752 -
        Flags: review?(alwu) → review+

    
    

    
  
(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...
Flags: needinfo?(jyavenard)

    
    

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

    
    

    
  
(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.

    
  
Assignee: jyavenard → alwu

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
  
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.

    
    

    
  
(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!
Flags: needinfo?(jyavenard)

    
    

    
  
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

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(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).

    
  

        Attachment #8909752 -
        Attachment is obsolete: true

    
  

        Attachment #8910667 -
        Flags: review?(jyavenard)

    
  

        Attachment #8910668 -
        Flags: review?(jyavenard)

    
    

    
  
(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.

    
    

    
  
(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.

    
    

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

    
    

    
  
(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 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

        Attachment #8910667 -
        Flags: review?(jyavenard) → review+

    
    

    
  
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

        Attachment #8910668 -
        Flags: review?(jyavenard)

    
    

    
  
(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 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.

        Attachment #8910668 -
        Flags: review?(jyavenard) → review+

    
    

    
      
  

    
    

    
  
Pushed by alwu@mozilla.com:
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/aabb5872e828
https://hg.mozilla.org/mozilla-central/rev/d7435cf7c4e7
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox58:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1402681

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: