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

Support media resources with multiple media tracks #22301

Closed
ferjm opened this issue Nov 28, 2018 · 3 comments
Closed

Support media resources with multiple media tracks #22301

ferjm opened this issue Nov 28, 2018 · 3 comments

Comments

@ferjm
Copy link
Member

@ferjm ferjm commented Nov 28, 2018

Spec: https://html.spec.whatwg.org/multipage/media.html#media-resources-with-multiple-media-tracks

This involves implementing the AudioTrackList and VideoTrackList interfaces and the logic of the audioTracks and videoTracks attributes.

@sreeise
Copy link
Contributor

@sreeise sreeise commented Jan 1, 2019

@ferjm Hello, if you don't mind, I would like to take this on.

@jdm
Copy link
Member

@jdm jdm commented Jan 2, 2019

You will want to follow the steps from https://doc.servo.org/script/dom/index.html#adding-a-new-dom-interface when adding the new interfaces.

@sreeise
Copy link
Contributor

@sreeise sreeise commented Jan 3, 2019

You will want to follow the steps from https://doc.servo.org/script/dom/index.html#adding-a-new-dom-interface when adding the new interfaces.

Gotcha, thanks for the heads up.

@ferjm ferjm moved this from To do to In progress in Media playback Jan 8, 2019
@jdm jdm added the C-has open PR label Jan 16, 2019
@jdm jdm added the C-assigned label Feb 14, 2019
bors-servo added a commit that referenced this issue Feb 28, 2019
Added AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList inte…

…rfaces

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

Added interfaces and webidl's for AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList to support media resources with multiple media tracks.

The pull request is not complete. There are a couple missing parts shown below and as well as tests. There are a few areas that are unclear to me and so I wanted to get some feedback on this. I appreciate your guys help :)

I wasn't sure where to add the steps for the audio and video track specific parts for the media data processing steps list:

https://html.spec.whatwg.org/multipage/media.html#loading-the-media-resource:audiotracklist

It looks like this should be added in handle_player_event() in HTMLMediaElement.rs. This method implements a part of the data processing steps list, but this wasn't completely clear to me.

Along with the place to add the steps where is the audio and video specific data coming from to check for and add these tracks? The metadata in handle_player_event() PlayerEvent::MetadataUpdated has two string vectors labeled audio_tracks and video_tracks from servo_media, however, I am not if this is the correct place to check for the tracks or whether the vectors hold the correct data?.

There are two places that look like it will require support from areas that have not been implemented in TrackEvent and Media fragment syntax. The last part is two places where the tracks are removed in the HTMLMediaElement which I need to add.

The build has no errors but does have warnings for unused methods for the parts mentioned above. Thanks again guys, and I'll be glad to fix the whatever is wrong :)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./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: -->
- [ ] 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 added a commit that referenced this issue Mar 1, 2019
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 added a commit that referenced this issue Mar 3, 2019
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 added a commit that referenced this issue Mar 3, 2019
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 -->
Media playback automation moved this from In progress to Done Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.