Multitrack audio video text rebased #691
Conversation
|
Update.. My CI nightly build machine is down so the demo will be up in the morning. |
… multitrack demo page
…n return tracks from mpd
…to MediaPlayer API
…s for multiple adaptation support. I will need to carefully picks and hand merge
Add ability to sort tracks and introduced new TextTrackInfo vo to make multi track sorting easy.
…why it breaks text tracks.
dsparacio
force-pushed
the
multitrack-audio-video-text-rebased
branch
from
889d87a
to
58f1959
Compare
|
@bbcrddave - I pulled in ALL the active PR related and now everything works as expect. Thanks a bunch for work and the feedback. Please review this change if you have time. Ill un-merged until next week. |
Fixed issue found when trying to hide captions and then turn back on.
| createTextTrackFromMediaInfo(null, this.mediaInfos[i]); | ||
| } | ||
| self.videoModel.getElement().textTracks.addEventListener('change', onTextTrackChange.bind(self)); | ||
| this.timescale = fragmentExt.getMediaTimescaleFromMoov(bytes); |
There was a problem hiding this comment.
Timescale can be NaN - it is used in a number of calculations which could subsequently return NaN. addCaptions definitely doesnt check for this, there may be other places too.
I note that you've actually improved on the existing code by doing something sensible in getMediaTimescaleFromMoov, but it's worth considering.
There was a problem hiding this comment.
I believe the timescale always returns NaN unless it is the initial segment in which I extract the value only once at that time. Not sure what a better way to do this would be. Any suggestions.
Also talking to EBU about EBU-TT it seems that we should use DTS not CTS for the start times. Any opinion on this topic?
There was a problem hiding this comment.
Yes, I think it should be DTS.
There was a problem hiding this comment.
Agreed it is DTS.
However I see no advantage for this application in using composition time offsets in the BMFF, so it's likely that ctts=0, in which case DTS=CTS. I wouldn't rely on ctts being zero though - just use DTS.
|
@AkamaiDASH, great work! I've stuck a few comments inline but on the whole this is awesome. |
|
@bbcrddave - Thank you so much for the in-depth code review and comments!! Addressing some now. |
Also cache parser instance locally (only for fragmented text) to avoid going to system each chunk to get TTMLParser… Few vars set back to default in abort as well.
…ebased Multitrack audio video text rebased
| @@ -270,6 +278,8 @@ MediaPlayer = function (context) { | |||
| doAutoPlay.call(this); | |||
| } | |||
| } | |||
|
|
|||
| this.adapter.reset(); | |||
There was a problem hiding this comment.
@AkamaiDASH @KozhinM This line is causing some problems for me. We are resetting the manifest adapter after we call doAutoPlay(). This causes the adapter to be reset after it has been initialized with data from the new manifest. Can I move this line into the if (!resetting) block so that it is only reset when the reset of the system objects are reset?
There was a problem hiding this comment.
@greg80303, let me explain why I put adapter.reset in this method - MediaPlayer now has getStreamsFromManifest and getTracksForTypeFromManifest methods that use DashAdapter's methods that in turn initialize adapter's periods and adaptations members. If these methods are called for a manifest A and then MediaPlayer.attachSource is called for a manifest B w/o resetting the adapter it will have incorrect data about periods and adaptations. That is why I reset adapter in resetAndPlay.
I think we need to move adapter.reset right at the top of the resetAndPlay function Since 'playing' and 'streamControllers' may not be set in case I described above. Does it make sense?
There was a problem hiding this comment.
@KozhinM I'm fine with moving it to the top. As long as it is called before doAutoPlay.call(this).
| } | ||
| }else { | ||
| streamProcessor.updateMediaInfo(manifest, allMediaForType[0]); |
There was a problem hiding this comment.
@AkamaiDASH, I believe this line breaks the adaptation switch process since it forces the player to always pick the first adaptation instead of the one passed as an argument. I created a PR with a fix, but before merging it I want to get your confirmation that it does not confront with your intentions that I am not aware of:
#700
After review, if you think that everything is fine, could you please merge the PR since the fixes that it includes are quite important.
This is a PR that includes all of PR #662, as well as the new multi text track support rebased on the current dev branch as of this morning.
There was one conflict that caused multi text support to fail
b6fd76b
so I reverted one line of code of which the resolution is being actively discussed
0355ce7 & #690.
@bbcrddave now we can fix this on top of the multi track work....
I will be mounting a version of our support player using video.js with mutli-text-track samples that will demo both external xml and fragmented text tracks. It can be found here:
http://mediapm.edgesuite.net/dash/private/support-player/nightly/v1.5/index.html
Once merged, I will submit another PR with a simple HTML sample as well as update the ref player index.html
PS The above demo url may not be up until later tonight or in morning.