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

CEA-608 rendering broken #1687

Closed
TobbeEdgeware opened this issue Dec 6, 2016 · 11 comments
Closed

CEA-608 rendering broken #1687

TobbeEdgeware opened this issue Dec 6, 2016 · 11 comments
Labels
Milestone

Comments

@TobbeEdgeware
Copy link

The rendering of Closed Captioning CEA_608 is broken (since 2.2.1, it seems).
The mixed samples included in the reference player renders both TTML and CEA-608 at the same time. The URL is http://vm2.dashif.org/dash/vod/testpic_2s/cea608_and_segs.mpd

The plain CEA-608 sample (http://vm2.dashif.org/dash/vod/testpic_2s/cea608.mpd) is working better, but the rendered size is too small and does not scale when going to full screen.

I hope to fix this for 2.4, but if I don't manage it may be better to include the plain samples instead of the mixed cases.

@TobbeEdgeware TobbeEdgeware added this to the v2.4.0 milestone Dec 6, 2016
@dsparacio
Copy link
Contributor

OK thanks @TobbeEdgeware. We can try to push into 2.4 OR we can do a one off point release before the end of the year after we discuss the release at the Face to Face. Look forward seeing you next week. Safe Travels. Keep me posted if you fix this before Friday.

@TobbeEdgeware
Copy link
Author

TobbeEdgeware commented Dec 7, 2016

@AkamaiDASH I've tracked down the issues causing the problems.

The mixture of TTML and CEA-608 is caused by your changes to the ControlBar between commit 1a11e2c and 42316b6 where you made a common mechanism for audio and captions menu (Sep. 1). As 1a11e2c works, it sets the language to the first CC track, while 42316b6 both starts the first CC track and the first TTML language. I cannot see directly how to fix it, but maybe you can?

The other issue is more recent 3bb4adf where a change to the TTML parser and rendering was done without modifying the CEA-608 which uses the same rendering. I have made PR #1690 for that and also added two test sources which have CEA-608 without TTML tracks.

@dsparacio
Copy link
Contributor

Hmm, but the changes should just se the API for setting the text track regardless of the change. I will have to dig into this one I get some time. But if is the control bar should not be a show stopper and that is something we can fix after a release and do a point release on. Ill try to find time.

@dsparacio
Copy link
Contributor

@TobbeEdgeware I see the White subtitle text top center and the Colored moving caption. When I switch to caption via the menu I notice it resolves the issue so looks like just at startup. Can you confirm you see the same.

URL
http://vm2.dashif.org/dash/vod/testpic_2s/cea608_and_segs.mpd

@TobbeEdgeware
Copy link
Author

@AkamaiDASH Yes, I see the same behavior, somehow the startup is two modes, but once I do an active choice, I only see one.

@dsparacio
Copy link
Contributor

I will fix this sometime today ;)

@dsparacio
Copy link
Contributor

So there is an easy fix in the controlbar but I am not convinced it is correct. If I remove the controlbar completely from a dash.js player it still happens. It is not setting A single track as the default track by hiding/disabling the other tracks. We need to fix this in Dash.js in my opinion. It is only when there is mixture. I bet, have not looked yet it seeing two type and setting default track for TTML and default track for embedded. Not sure yet.

Code to fix in control bar (brute force)

 if (type === undefined) { .........
else if (type === 'caption-list') {
    player.setTextTrack(0);
}

@dsparacio
Copy link
Contributor

Root of the issue is these lines in TextTrack. Some race. If I set a break point and hold for a sec it works as expected. Ill figure a fix out for this asap

setCurrentTrackIdx.call(this, defaultIndex);
            if (defaultIndex >= 0) {
                video.textTracks[defaultIndex].mode = 'showing';
                this.addCaptions(defaultIndex, 0, null);
            }

@dsparacio
Copy link
Contributor

We need to discuss the desired default selection of text tracks. I believe the logic in this block is failing with the first item being ttml not embedded. So we do not hide the first track during initialization. For this particular manifest we are setting track 1 as default not track 0 due to logic below. I think it is a simple fix but not sure what the exact desired behavior should be. Maybe we can discuss at Face to Face and resolve then.

function getIsDefault(mediaInfo) {
        //TODO How to tag default. currently same order as listed in manifest.
        // Is there a way to mark a text adaptation set as the default one? DASHIF meeting talk about using role which is being used for track KIND
        // Eg subtitles etc. You can have multiple role tags per adaptation Not defined in the spec yet.
        var isDefault = false;
        if (embeddedTracks.length > 1) {
            isDefault = (mediaInfo.id && mediaInfo.id === 'CC1'); // CC1 if both CC1 and CC3 exist
        } else if (embeddedTracks.length === 1) {
            if (mediaInfo.id && mediaInfo.id.substring(0, 2) === 'CC') {// Either CC1 or CC3
                isDefault = true;
            }
        } else {
            isDefault = (mediaInfo.index === mediaInfos[0].index);
        }
        return isDefault;
    }

@TobbeEdgeware
Copy link
Author

I agree that it is not obvious what to do, but whatever we do, it should be well-defined and only one visible track. Let's have a chat about it during the f2f.

dsparacio pushed a commit to dsparacio/dash.js that referenced this issue Dec 16, 2016
dsparacio pushed a commit that referenced this issue Dec 16, 2016
@dsparacio
Copy link
Contributor

fixed with #1716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants