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

Use internal slots for various MediaSource and SourceBuffer object state #295

Merged
merged 9 commits into from
Sep 22, 2021

Conversation

wolenetz
Copy link
Member

@wolenetz wolenetz commented Sep 3, 2021

Note, track buffers and their internal state remain as "variables", not internal slots, in the spec for now. Later refactoring might make them internal slots too, though at lower priority.

This partially fixes #286


Preview | Diff

In eventual squashed commit, reference w3c#286.
Also adds a reference to issue w3c#289 in the coded frame eviction
algorithm.
Also adds text in the definition that mentions that `changeType()`
updates `generate timestamps flag`.

Note, this dfn is exported, so any previous external references to it via
[=generate timestamps flag=] might need to be changed to
{{SourceBuffer/[[generate timestamps flag]]}} wherever it is referenced.

In particular, media-source.js is updated to (hopefully) enable correct
fragment reference for this definition from MSE BSF Registry ED. The
longer-term fix will be to update that registry's respec directly to
use xref and the linkage, above.
@wolenetz
Copy link
Member Author

wolenetz commented Sep 3, 2021

@tidoust / @mwatson2 - Am I correct that the current external reference by the BSF Registry of generate timestamps flag via the helpers in media-source.js would be more stable if instead that registry used xref to reference the media-source spec, and {{SourceBuffer/[[generate timestamps flag]]}} respec syntax to reference the definition of that internal slot? For now at least, I think I've updated media-source.js in this PR to use the updated fragment for this former variable that is now an internal slot.

Also, please review soon if you could, as rebasing this could get ugly since it touches quite a number of lines. Thanks!

Copy link
Member

@tidoust tidoust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me! The need to write terms surrounded by [[ ]] and {{ }} in the source spec is not necessarily fantastic from an authoring perspective. Hopefully, the ability to better track and validate references to these terms makes it acceptable.

@tidoust / @mwatson2 - Am I correct that the current external reference by the BSF Registry of generate timestamps flag via the helpers in media-source.js would be more stable if instead that registry used xref to reference the media-source spec, and {{SourceBuffer/[[generate timestamps flag]]}} respec syntax to reference the definition of that internal slot?

Yes and you were right to flag the definition of the internal slot with a data-export attribute so that it can be referenced by the BSF registry.

@wolenetz
Copy link
Member Author

@mwatson2 - please take a look, or is @tidoust's prior approval sufficient? Thanks!

Copy link
Contributor

@mwatson2 mwatson2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The only thing I noticed - which exists in the pre-existing text and so is not a comment on this PR - is that some of these variables have explicitly stated types (boolean / TimeRanges) and some do not.

@wolenetz
Copy link
Member Author

wolenetz commented Sep 22, 2021

Looks good! The only thing I noticed - which exists in the pre-existing text and so is not a comment on this PR - is that some of these variables have explicitly stated types (boolean / TimeRanges) and some do not.

Thanks for the review. I too noticed some types missing. Some are simple, others like the "track buffer" concept have less strongly typed information, as it is left more to implementations. Possibly this is an area of improvement in the spec though, especially if things like buffered media introspection API is landed (where the notion of an app-visible "track buffer" object might make sense -- I've added a note around this to #259 ). - Edit: and I've also filed #297

@wolenetz wolenetz merged commit 8842ce9 into w3c:main Sep 22, 2021
github-actions bot added a commit that referenced this pull request Sep 22, 2021
…ate (#295)

SHA: 8842ce9
Reason: push, by @wolenetz

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@wolenetz wolenetz deleted the use_internal_slots branch September 22, 2021 19:41
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.

Editorial: Modernize to use internal slots instead of "internal ... variable"
3 participants