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

Add correct support of audio element #2494

Conversation

nicosang
Copy link
Contributor

Hi,

this PR has to solve issue #2490. Unit tests have been also updated by adding checking of element type.
@VincentFTS, could you, please, take a look at this PR?

Many thanks,
Nico

@davemevans
Copy link
Contributor

This seems like a bit of a hack to me. Additionally, there doesn't seem to be a similar option if I want the player to ignore audio when playing in a video element. #2490 admits that the issue is caused when trying to hack round another problem. Further it looks like if you want to preload and attach a media element later, dash.js still preload all the video?

Ultimately, IMO, the top level application should only be passing audio-only manifests to the player when it knows the media element is of type audio.

That said, if this is a problem we want to solve in the general case, perhaps it would be better to have an explicit configuration option to ignore streams of a particular MIME type?

@nicosang
Copy link
Contributor Author

Hi @bbcrddave ,

there is many points that attempt to be fix with this PR. First of all, I think we do agree together to add tests on the type of element that is used by SetElement. The video element is able to play both audio and video, so, according to me, it is not useful to have same behavior than with the audio element.

You're right, in preload use case, we don't know if the element will be an audio or a video so, we can't filter correctly. So, indeed, a configuration option could be interesting to add....

Nico

@davemevans
Copy link
Contributor

For clarity, I meant the application controlling dash.js should know what the element type is, and only select appropriate content to be presented, rather than dash.js making the decision. That application is more likely to be aware of the context of the page, and so could either configure dash.js appropriately, or only select appropriate content.

In the past (#1309) we have removed code detecting the object type since it would break, for example, the use case where vendors create an <object> which has an API compatible with HTMLMediaElement. In this case, the nodeName would be OBJECT, which would not pass this test. It's probably less important more recently I guess.

Anyway, I don't feel that strongly - was just making the case for an alternative solution 😄

@VincentJousse
Copy link

@nicosang ,
thank you, it works !

@bbcrddave
Imagine a context where the application needs to play some film soundtracks, it would be a pity to have to create a specific manifest whereas the "original" film manifest contains everything needed.
I agree that making dash.js configurable about this may be an elegant solution ;)

@nicosang nicosang force-pushed the addAudioElementSupport branch 4 times, most recently from 04ec702 to 1728a7a Compare April 3, 2018 15:58
@epiclabsDASH epiclabsDASH added this to the 2.7 milestone Apr 28, 2018
@epiclabsDASH epiclabsDASH merged commit f44e558 into Dash-Industry-Forum:development May 16, 2018
@nicosang nicosang deleted the addAudioElementSupport branch May 17, 2018 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants