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

Added AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList inte… #22622

Merged
merged 1 commit into from Mar 3, 2019

Conversation

5 participants
@sreeise
Copy link
Contributor

sreeise commented Jan 5, 2019

…rfaces

Added AudioTrack, AudioTrackList, VideoTrack, VideoTrackList, and TrackEvent interfaces to support multiple media tracks.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22301 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@jdm jdm self-assigned this Jan 5, 2019

@jdm
Copy link
Member

jdm left a comment

Since the code for the audio and video tracks is so similar, please consider whether each comment made about the video track list implementation could be applied to the audio track list implementation. My one concern about this PR is that we're adding a bunch of code that does not get executed, since there are no callers for AudioTrackList::add and VideoTrackList::add. You can get the information about audio and video tracks from the Metadata value that's in PlayerEvent::MetadataUpdated.

Show resolved Hide resolved components/script/dom/videotracklist.rs Outdated
Show resolved Hide resolved components/script/dom/videotracklist.rs Outdated
}

pub fn is_empty(&self) -> bool {
self.tracks.borrow().is_empty().clone()

This comment has been minimized.

@jdm

jdm Jan 7, 2019

Member

The clone should not be necessary here.

}

pub fn set_selected(&self, idx: usize, value: bool) {
self.item(idx).unwrap().set_selected(value);

This comment has been minimized.

@jdm

jdm Jan 7, 2019

Member

We should probably silently ignore this instead of unwrapping.

self.tracks.borrow_mut().push(Dom::from_ref(track));
}
} else {
self.tracks.borrow_mut().push(Dom::from_ref(track));

This comment has been minimized.

@jdm

jdm Jan 7, 2019

Member

This looks like we can rewrite this as:

if track.selected() {
    if let Some(idx) = self.selected_index() {
        self.set_selected(idx, false);
    }
}
self.tracks.borrow_mut().push(Dom::from_ref(track));
} else {
self.tracks.borrow_mut().push(Dom::from_ref(track));
}
self.upcast::<EventTarget>().fire_event(atom!("addtrack"));

This comment has been minimized.

@jdm

jdm Jan 7, 2019

Member

According to the steps in https://html.spec.whatwg.org/multipage/media.html#media-data-processing-steps-list that create these tracks, the events are supposed to be TrackEvent objects, not just simple Event objects.

self.tracks
.borrow()
.iter()
.filter(|track| track.id() == id)

This comment has been minimized.

@jdm

jdm Jan 7, 2019

Member

This can use find instead of filter & next.

Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
Show resolved Hide resolved components/script/dom/htmlmediaelement.rs Outdated
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 7, 2019

Welp, I totally forgot to read the long useful description in the pull request that you provided, but I think we came to similar conclusions.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 7, 2019

As for the contents of those vectors that you mentioned, the string values are the names of the codecs associated with those media tracks. We can add a TODO comment about using the codec information to choose the optimal track to enable.

@sreeise

This comment has been minimized.

Copy link
Contributor Author

sreeise commented Jan 7, 2019

Welp, I totally forgot to read the long useful description in the pull request that you provided, but I think we came to similar conclusions.

No worries! You actually answered all of questions I had and was actually going to ask now so it works for me. Thanks for the helpful information and ill get back with a much cleaner pr soon :).

@ferjm ferjm added this to In progress in Media playback Jan 10, 2019

@sreeise

This comment has been minimized.

Copy link
Contributor Author

sreeise commented Jan 14, 2019

@jdm
Hey Josh, I am trying to figure out how to update the tracks correctly but unsure on this. When looking at the metadata from here:

PlayerEvent::MetadataUpdated(ref metadata) => {

It seems that only the metadata.format would be relevant (besides the codecs you mentioned) from this part? I was trying to use GDB to figure out what exactly the metadata format can look like to determine the best route but could not debug it successfully. Is the format showing the type such as "audio" or "video" or is there something more specific here?

Also, is the correct way to retrieve the rest of the track values by getting the attribute with the local_name! macro? Something like:

element.get_string_attribute(&local_name!("kind"));

Thanks for the help!

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 16, 2019

The format contains a string that corresponds with https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/GstPlayerMediaInfo.html#gst-player-media-info-get-container-format, which doesn't give any meaningful information about what it might contain. I'm not sure that it will be useful here.

If the media tracks have a kind content attribute, then that would be the appropriate way to retrieve if it element is an HTMLTrackElement (or an Element that is actually an HTMLTrackElement under the hood).

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 17, 2019

☔️ The latest upstream changes (presumably #22692) made this pull request unmergeable. Please resolve the merge conflicts.

@sreeise sreeise force-pushed the sreeise:audio_video_tracks branch from 6707d45 to 5507362 Jan 30, 2019

@sreeise sreeise force-pushed the sreeise:audio_video_tracks branch from 5507362 to 652f949 Jan 30, 2019

@sreeise

This comment has been minimized.

Copy link
Contributor Author

sreeise commented Jan 30, 2019

Hello, I am sorry for getting back to this so late. I added a part of the media processing steps but I am currently stuck on the kind attribute. I added an inline comment discussing the video and audio formats and how they are determined for this.

Thanks for the help.

@sreeise sreeise force-pushed the sreeise:audio_video_tracks branch from 652f949 to b4c8b40 Jan 30, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 31, 2019

error: unused import: `crate::dom::htmlaudioelement::HTMLAudioElement`
  --> components/script/dom/htmlmediaelement.rs:32:5
   |
32 | use crate::dom::htmlaudioelement::HTMLAudioElement;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
@jdm
Copy link
Member

jdm left a comment

This is very close!

}

pub fn is_empty(&self) -> bool {
self.tracks.borrow().is_empty().clone()

This comment has been minimized.

@jdm

jdm Jan 31, 2019

Member

No need to clone this value.

event_handler!(onaddtrack, GetOnaddtrack, SetOnaddtrack);

// https://html.spec.whatwg.org/multipage/#handler-tracklist-onremovetrack
event_handler!(onremovetrack, GetOnremovetrack, SetOnremovetrack);

This comment has been minimized.

@jdm

jdm Jan 31, 2019

Member

Remove the on prefix from the first argument for event handlers.

}

// Steps 7
// Fire an event named addtrack at this AudioTrackList object, using TrackEvent

This comment has been minimized.

@jdm

jdm Jan 31, 2019

Member

Can we implement this step in this PR?

}

// Steps 7
// Fire an event named addtrack at this VideoTrackList object, using TrackEvent

This comment has been minimized.

@jdm

jdm Jan 31, 2019

Member

Can we implement this step in this PR?

self.set_selected(idx, false);
}
}
self.tracks.borrow_mut().push(Dom::from_ref(track));

This comment has been minimized.

@jdm

jdm Jan 31, 2019

Member

We should push this before calling set_selected, otherwise the event handler for the change event could see an unexpected intermediate state.

event_handler!(onaddtrack, GetOnaddtrack, SetOnaddtrack);

// https://html.spec.whatwg.org/multipage/#handler-tracklist-onremovetrack
event_handler!(onremovetrack, GetOnremovetrack, SetOnremovetrack);

This comment has been minimized.

@jdm

jdm Jan 31, 2019

Member

Remove the on prefix from the first argument to these event handlers.

Show resolved Hide resolved components/script/dom/audiotracklist.rs
if let Some(track) = self.item(idx) {
track.set_selected(value);
}
self.upcast::<EventTarget>().fire_event(atom!("change"));

This comment has been minimized.

@jdm

jdm Jan 31, 2019

Member

Per https://html.spec.whatwg.org/multipage/media.html#toggle-audio-track, this needs to be a queued task like

task_source
.queue(
task!(resolve_pending_play_promises: move || {
let this = this.root();
if generation_id != this.generation_id.get() {
return;
}
. Also, we should only queue the task if an actual change occurred.

@sreeise sreeise force-pushed the sreeise:audio_video_tracks branch from b4c8b40 to fa722a7 Feb 4, 2019

@sreeise sreeise force-pushed the sreeise:audio_video_tracks branch from 4c79042 to 094777c Mar 3, 2019

@sreeise

This comment has been minimized.

Copy link
Contributor Author

sreeise commented Mar 3, 2019

Hopefully the changes are correct. I am having an issue testing these locally. When I try run tests/wpt/web-platform-tests/html/dom/interfaces.https.html it crashes with this error:

pid:7447 VMware, Inc.
 0:03.95 pid:7447 softpipe
 0:03.95 pid:7447 3.3 (Core Profile) Mesa 18.3.0-devel
 0:03.96 pid:7447 assertion failed: `(left == right)`
 0:03.96 pid:7447   left: `1282`,
 0:03.96 pid:7447  right: `0`

which begins at .cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.6.8/src/gl.rs:93

The other thing I noticed on development builds, which happened before I changed the test expectations, is this warning:

 --> components/script/dom/bindings/conversions.rs:74:5
   |
74 |     rustc_on_unimplemented = "The IDL interface `{Self}` is not derived from `{T}`."
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_attributes)] on by default

I am not sure what the cause of these are. They could just be happening for me locally.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 3, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 3, 2019

📌 Commit 094777c has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 3, 2019

⌛️ Testing commit 094777c with merge 543bf19...

bors-servo added a commit that referenced this pull request Mar 3, 2019

Auto merge of #22622 - sreeise:audio_video_tracks, r=jdm
Added AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList inte…

…rfaces

<!-- Please describe your changes on the following line: -->

Added AudioTrack, AudioTrackList, VideoTrack, VideoTrackList, and TrackEvent interfaces to support multiple media tracks.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22301  (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22622)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 3, 2019

💔 Test failed - linux-rel-wpt

@sreeise sreeise force-pushed the sreeise:audio_video_tracks branch from 094777c to cac4aa5 Mar 3, 2019

@sreeise

This comment has been minimized.

Copy link
Contributor Author

sreeise commented Mar 3, 2019

Yep I missed one, sorry!. Hopefully this one will finally pass.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 3, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 3, 2019

📌 Commit cac4aa5 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 3, 2019

⌛️ Testing commit cac4aa5 with merge 4ccbd1e...

bors-servo added a commit that referenced this pull request Mar 3, 2019

Auto merge of #22622 - sreeise:audio_video_tracks, r=jdm
Added AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList inte…

…rfaces

<!-- Please describe your changes on the following line: -->

Added AudioTrack, AudioTrackList, VideoTrack, VideoTrackList, and TrackEvent interfaces to support multiple media tracks.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22301  (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22622)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 3, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: jdm
Pushing 4ccbd1e to master...

@bors-servo bors-servo merged commit cac4aa5 into servo:master Mar 3, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

Media playback automation moved this from In progress to Done Mar 3, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 3, 2019

Woohoo! Thanks for sticking with this!

@sreeise sreeise deleted the sreeise:audio_video_tracks branch Mar 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.