-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add support for Remote Playback API (and some cleanup) #134
Conversation
pthatcherg
commented
Jan 5, 2019
- Add the protocol message for Remote Playback
- Add the mapping from API to messages
- Cleanup some terminology used by the Presentation API to make the two be in sync
- Switch everything to using the "definition list" of bikeshed, which seems like a better fit
And also cleanup: - key/value lists to use better format - links to document sections better
controller may send a remote-playback-availability-request message with the | ||
following values: | ||
|
||
: urls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I'm wrong, but wouldn't we always be querying with a single URL (URL of the media element)?
https://w3c.github.io/remote-playback/#dom-remoteplayback-watchavailability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Media elements can have multiple resource URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me, I have some spec questions as well as minor grammar nits. Thanks for putting this together!
index.bs
Outdated
5 : bool ; selected | ||
} | ||
|
||
track-track-state = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure some of this is my unfamilliarity with the spec best practices, but why wouldn't we just have mode as part of track-state above?
- Are there instances where we would have track-state but not track-track state?
- Do audio or video track-states also have modes? Always? Sometimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be text-track-state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it was a typo. I changed it to text-track-state.
|
||
media-error = [ | ||
code: &( | ||
user-aborted: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful to have an error code for an internal crash, e.g. something not necessarily decode or network? Or does that scenario not happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I added unknown-error for now
index.bs
Outdated
} | ||
|
||
remote-playback-state = { | ||
? 1: (rate: bool, preload: bool, poster: bool, added-text-track: bool, added-cues: bool) ; supports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also use the ampersand syntax below? I'm not familiar with this markup language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I added it.
} | ||
|
||
; type key 118 | ||
remote-playback-termination-response = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type isn't used anywhere. Delete? I don't think we need a response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, but I had a typo where I put in the wrong name for the message. I fixed it, so you should be able to see it now.
availability request. | ||
|
||
To save power, the controller may disconnect the QUIC connection and | ||
later reconnect to send availablity requests and receive availability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling mistake on this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
[HtmlMediaElement.played](https://html.spec.whatwg.org/multipage/media.html#dom-media-played). | ||
|
||
: seekable-time-ranges | ||
:: The time ranges for which media is seekable (ranges the controller or the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: can we rephrase without a parenthetical insert?
E.g.
:: The time ranges the controller or receiver may seek to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
index.bs
Outdated
[HtmlMediaElement.videoHeight](https://html.spec.whatwg.org/multipage/media.html#dom-media-videoheight). | ||
|
||
: audio-tracks | ||
:: The audio tracks available, which can individually enabled or disabled. See |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar nit: here and below, the syntax "The <foo> available" is usually phrased as "The available <foo>."
Consider:
:: The available audio tracks. Each track can be individually enabled or disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
index.bs
Outdated
[HtmlMediaElement.audioTracks](https://html.spec.whatwg.org/multipage/media.html#dom-media-audiotracks) | ||
|
||
: video-tracks | ||
:: The video tracks available, one of which can be selected. See |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar nit: Is it exactly zero or one tracks that can be selected? Would be cleaner as:
:: The available video tracks. Only one track may be selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
index.bs
Outdated
[HtmlMediaElement.videoTracks](https://html.spec.whatwg.org/multipage/media.html#dom-media-videotracks). | ||
The controller can also add cues to and remove cues from text tracks. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: extra newlines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
remote-playback-modify-response and remote-playback-state-event messages to | ||
change the local media element based on changes to the remote playback state. | ||
|
||
<!-- TODO: Have a very descriptive, precise algorithm for what messages to send |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we track these TODOs: as issues in the GitHub repository? I think a separate patch converting the TODOs to issues would be handy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should have issues for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great. I think there are some areas where the remote playback synchronization could be clarified, but most comments are minor editing suggestions. It might make sense to respond to the high level feedback and land as-is, then one of us can follow up with a copy editing cleanup PR.
presentation request URL. | ||
We borrow terminology from the [[PRESENTATION-API|Presentation API]]. We call | ||
the agent that is used to discover and initiate presentation of Web content on | ||
another device the [=controller=] (or [=controlling user agent=] when it is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The controller is the application that is requesting presentation, not the agent. If you want to redefine it here, then please be specific that this is a different definition than the API's. But we'd then want to go back and change the API to prevent rampant confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a protocol perspective, it's the browser that's the controller, even if it's doing things on behalf of a "browsing context" that's the controller behind the curtains. I can try and make this clear in this text.
the agent that is used to discover and initiate presentation of Web content on | ||
another device the [=controller=] (or [=controlling user agent=] when it is a | ||
browser). We call the agent on the device rendering the Web content the | ||
[=receiver=] or [=presentation display=] (or [=receiving user agent=] when it is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just use receiver for the agent that answers requests and renders presentations.
What is the distinction between "receiver" and "receiving user agent"? Shouldn't every agent be a user agent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A receiver could potentially launch a non-user agent. Maybe it has specific native code to use for certain URLs that aren't user agents.
If you want to change the term Remote Playback "receiver" into something other "receiver", could we debate what that term should be and change it in a follow-up PR? It might take a while to pick a different noun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a github issue to remember to discuss this (and also the word "controller")
[[PRESENTATION-API|Presentation API]]. A subsequent section will | ||
define how APIs in [[PRESENTATION-API|Presentation API]] map to the | ||
terminating, and controlling presentations as defined by | ||
[[PRESENTATION-API|Presentation API]]. [[#presentation-api]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need anchor text for #presentation-api to render something useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of places where this style is used in this doc, so I think it works.
index.bs
Outdated
receiving updates about the URLs, should the availability change. | ||
: watch-duration | ||
:: The period of time that the controller is interested in receiving updates | ||
about the URLs, should the availability change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the/their/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
index.bs
Outdated
- watch-id: An identifier the receiver may use when sending updates about URL | ||
availability so that controller knows which URLs the receiver is referring | ||
: watch-id | ||
:: An identifier the receiver may use when sending updates about URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this message meaningful to the controller without a watch-id? If not, make this a "should" or "must".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it "must".
index.bs
Outdated
message, it is assumed there is no such major error. | ||
|
||
: epoch | ||
:: The "zero time" of the media timeline. See |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Units?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
media-time
index.bs
Outdated
:: The "zero time" of the media timeline. See | ||
[HtmlMediaElement's timeline offset](https://html.spec.whatwg.org/multipage/media.html#timeline-offset) and | ||
[HtmlMediaElement.getStartDate()](https://html.spec.whatwg.org/multipage/media.html#dom-media-getstartdate). | ||
If not present in the intial state in the remote-playback-start-response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to accommodate clock skew between the sender and receiver? Otherwise it seems like the timelines are going to be out of sync w.r.t. actual, wall clock time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wall clock times will always be out of sync between sender and receiver (there is no solution to clock sync). I think the controller will have to deal with that however it sees fit.
index.bs
Outdated
message, it is assumed to be unknown. | ||
|
||
: duration | ||
:: The duration of the media timeline. See |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Units?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
media-time
index.bs
Outdated
:: The duration of the media timeline. See | ||
[HtmlMediaElement.duration](https://html.spec.whatwg.org/multipage/media.html#dom-media-duration). | ||
If not present in the intial state in the remote-playback-start-response | ||
message, it is assumed to be unknown. For unbounded streams, it may be "forever". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "forever" a special numeric value (NaN)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's more like a CDDL type union.
index.bs
Outdated
|
||
: text-tracks | ||
:: The text tracks available, which can be individually shown, hidden, or disabled. See | ||
[HtmlMediaElement.videoTracks](https://html.spec.whatwg.org/multipage/media.html#dom-media-videotracks). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/videoTracks/textTracks/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done