Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make buffered/played/seekable attributes of media element bindable #819

Conversation

martinandert
Copy link
Contributor

I need this for a little side project of mine.

It's hard to add tests as these three IDL attributes are readonly (the spec for testing the existing currentTime and duration attributes is skipped for that same reason), but happy to provide a REPL/gist example once it's merged.

@martinandert
Copy link
Contributor Author

Not sure why CI fails. Looks like an unrelated issue.

@Rich-Harris
Copy link
Member

Oh, I think the tests are failing because some stuff is out of date, and is fixed in #815. I'll merge that and we can try again.

Thanks for the PR! I haven't dealt with media events in a little while, so this is likely a misunderstanding on my part, but is the requestAnimationFrame stuff necessary for these events?

@martinandert
Copy link
Contributor Author

[...] is the requestAnimationFrame stuff necessary for these events?

Yes, it's necessary when rendering the supplied time ranges in the UI. Without it, the browser probably can't cope with all the updates and the audio starts cracking.

@Rich-Harris
Copy link
Member

I've had some time to look at this a bit more closely. My feeling is that we probably want to avoid requestAnimationFrame as far as possible — at present, if you have bind:buffered, you get an update 60 times a second whether or not the media is pending, loading or fully loaded. It seems that the progress event should be enough, no?

We could probably update seekable on the progress event as well — my understanding is that it basically matches buffered unless range requests are supported, in which case it's basically just the full duration of the media from the get-go.

As for played, that one probably does belong inside a requestAnimationFrame callback while playback is happening, and in fact we could probably piggyback on bind:currentTime (i.e. use a single callback for both bindings, if both are present).

A nice optimisation might be to only update when the time ranges have actually changed (though avoiding requestAnimationFrame would get rid of most of the redundant updates), which leads me to a small rant about the DOM's media APIs: the TimeRanges object is nuts. I cannot imagine how a group of intelligent people could come up with something as completely batshit as this:

screen shot 2017-09-07 at 3 39 19 pm

The fact that audio.buffered !== audio.buffered is just blowing my mind. Anyway, since any piece of UI that depended on a TimeRanges object would have to convert it to an array anyway, I propose that our bindings do that automatically — so the ranges in the screenshot would become this:

[[0, 201.698]]

From there, it would be relatively straightforward to avoid another update if the next set of ranges is also [[0, 201.698]].

What do you reckon?

@martinandert
Copy link
Contributor Author

martinandert commented Sep 9, 2017

[...] at present, if you have bind:buffered, you get an update 60 times a second whether or not the media is pending, loading or fully loaded. It seems that the progress event should be enough, no?

It's already using the progress event for buffered. You probably overlooked that.

We could probably update seekable on the progress event as well [...]

Yeah I think that's a good optimization.

As for played, that one probably does belong inside a requestAnimationFrame callback while playback is happening, and in fact we could probably piggyback on bind:currentTime (i.e. use a single callback for both bindings, if both are present).

Also agreed.

[...] a small rant about the DOM's media APIs: the TimeRanges object is nuts [...]

They basically confirmed that in their spec.

[...] since any piece of UI that depended on a TimeRanges object would have to convert it to an array anyway [...]

Actually I don't do that. Please see this gist for an example. Click on the progressbar to see the played and buffered ranges changing. The seekable ranges (only one as you pointed out) are rendered transparent, but they are used as click targets for seeking. The gist will also work in the REPL if the changes of this PR are applied.

What do you reckon?

I'm not sure. How do you think we shall go on from here?

@Rich-Harris
Copy link
Member

Rich-Harris commented Sep 9, 2017

Didn't overlook — just saying you don't need progress and rAF. progress should be sufficient as far as I can tell.

Actually I don't do that.

Yeah, you kind of do 😀 In your TimeRanges component, you're creating a this.ranges array of TimeRange components, and manually appending them to the DOM with refs. And then, to keep them up to date, you have to do things like this...

updateColor(color) {
  this.ranges.forEach((range) => range.set({ color }));
}

...because you can't use <TimeRange> declaratively. If the binding value was an array of arrays (or array of {start, end} objects — whichever), then your entire TimeRanges component would look like this instead of this:

<div>
  {{#each ranges as range}}
    <TimeRange start={{range.start}} end={{range.end}} :color :duration :cursor on:seek/>
  {{/each}}
</div>

<style>

  div {
    position: absolute;
    left: 0;
    top: 0;
    width: 100%;
    height: 100%;
  }

</style>

<script>

  import TimeRange from './TimeRange.html';

  export default {
    components: {
      TimeRange,
    },

    data() {
      return {
        duration: NaN,
        list: [],
        cursor: 'auto',
      };
    }
  };

</script>

That seems like an unambiguous win.

How do you think we shall go on from here?

Let me poke around with this branch locally for a bit.

An aside — you don't need to bind observers, they're called with the component as context:

// these are equivalent
this.colorObserver = this.observe('color', this.updateColor.bind(this));
this.colorObserver = this.observe('color', this.updateColor); 

(Though that will be moot once these changes are made.)

@Rich-Harris Rich-Harris merged commit db0b6d7 into sveltejs:master Sep 9, 2017
@Rich-Harris
Copy link
Member

Shit, accidentally merged! Will open a new PR with the changes, once they're done

@martinandert martinandert deleted the bind-buffered-played-seekable-of-media branch September 10, 2017 13:00
Rich-Harris added a commit that referenced this pull request Sep 10, 2017
remove requestAnimationFrame stuff, convert time ranges to {start, end} objects
@fartinmartin
Copy link

Hello ~5 year old thread! I just stumbled across this PR after trying to understand when the svelte buffered binding was being updated. In my basic fiddling around it seems that the progress event doesn't actually fire frequently enough to reflect the state of the HTMLMediaElement.buffered's TimeRange object.

I made a REPL to help show what I mean. To me, it looks like updating buffered at the same rate as currentTime is much more reflective of the native TimeRange object. I could be way off, and am sure my REPL isn't the most performant, but am curious on y'all's thoughts and what the best way forward might be 🤠

What ever the case,

the TimeRanges object is nuts.

can't be said enough, lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants