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

Cut Closed captions (C608) when html tag mixed with normal text #11019

Closed
1 task
ZygmuntMalek opened this issue Feb 24, 2023 · 10 comments
Closed
1 task

Cut Closed captions (C608) when html tag mixed with normal text #11019

ZygmuntMalek opened this issue Feb 24, 2023 · 10 comments
Assignees
Labels

Comments

@ZygmuntMalek
Copy link

ZygmuntMalek commented Feb 24, 2023

ExoPlayer Version

2.17.1

Devices that reproduce the issue

All devices
I've tested at Samsung Galaxy A10 but our client report it at multiple devices

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Yes

Reproduction steps

It's reproducible at demo device but required is special content.
IMHO when measure space need to draw Closed captions 608, in case there is html mixed content
eg.

The USS Cerritos remains in
Spacedock, impounded for further
investigation.

Also this is a regression introduced between r2.11.8 => r2.12.3 and can be reproduced on newest version

Expected result

Issue were not observed eg. at Exoplayer r2.11.8

Actual result

Space for draw Closed captions is cut, and such subtitles are not displayed properly
Exoplayer 2.11.8

Exoplayer2 11 8

Exoplayer 2.12.3

Exoplayer2 12 3

Exoplayer 2.18.2

Exoplayer2 18 2

Exoplayer 2.7.3
Exoplayer 2 7 3

Media

I'm trying to get publicly accessed content but currently I'm waiting for it.

IMHO to reproduce issue there is need content with embed closed caption where html tagged eg. by Italic text fragment is mixed with normal text

Bug Report

  • You will email the zip file produced by adb bugreport to [email protected] after filing this issue.
@icbaker
Copy link
Collaborator

icbaker commented Mar 3, 2023

Please provide a file we can play that reproduces the issue - without this it's not really feasible for us to investigate I'm afraid.

Please either upload it here or send to [email protected] using the subject Issue #11019. Please also update this issue to indicate you’ve done this.

@google-oss-bot
Copy link
Collaborator

Hey @ZygmuntMalek. We need more information to resolve this issue but there hasn't been an update in 14 weekdays. I'm marking the issue as stale and if there are no new updates in the next 7 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

@google-oss-bot
Copy link
Collaborator

Since there haven't been any recent updates here, I am going to close this issue.

@ZygmuntMalek if you're still experiencing this problem and want to continue the discussion just leave a comment here and we are happy to re-open this.

@ZygmuntMalek
Copy link
Author

ZygmuntMalek commented May 5, 2023

@icbaker link to content sent in email

@google google locked and limited conversation to collaborators Jun 3, 2023
@icbaker icbaker reopened this Jun 5, 2023
@icbaker
Copy link
Collaborator

icbaker commented Jun 5, 2023

I had a look at the provided media. I copied the entire directory (maintaining the structure from the zip file) to my device and tried playing both the .ism and .ismc files as Smooth Streaming manifests, but ExoPlayer failed because it expected to find a /Manifest directory underneath the ism file. It looks like there might be some directory structure missing? I'm afraid I'm not going to try and recreate it by trial and error.

I looked at the 360p file directly with mediainfo and it thinks it contains CEA-608 subtitle data, but ExoPlayer doesn't find any tracks apart from the video one when it plays the file directly (skipping the manifests). I didn't see any subtitle-specific data in the manifests, but I'm not a Smooth Streaming expert so I might have missed it.

Can you please provide fuller repro steps of how to get the subtitles to display in our demo app?

@google-oss-bot
Copy link
Collaborator

Hey @ZygmuntMalek. We need more information to resolve this issue but there hasn't been an update in 14 weekdays. I'm marking the issue as stale and if there are no new updates in the next 7 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

@google google unlocked this conversation Jun 28, 2023
@ZygmuntMalek
Copy link
Author

Access to new content sent by email - it should be easiest to use it, I were able to reproduce issue using this content myself - if there will be need for more details I could pass detailed steps how to.

@icbaker
Copy link
Collaborator

icbaker commented Jun 29, 2023

Thanks - I'm able to reproduce the problem now.

I added some logging and I see the full text of the cue when the italic span is being added:

Cea608Decoder.setItalicSpan() builder='The USS Cerritos remains in', start=8, end=17

The end of the text gets lost in the truncate calls here (see #7341 for context on why we truncate here):

// The number of empty columns before the start of the text, in the range [0-31].
int startPadding = indent + tabOffset;
int maxTextLength = SCREEN_CHARWIDTH - startPadding;
SpannableStringBuilder cueString = new SpannableStringBuilder();
// Add any rolled up captions, separated by new lines.
for (int i = 0; i < rolledUpCaptions.size(); i++) {
cueString.append(Util.truncateAscii(rolledUpCaptions.get(i), maxTextLength));
cueString.append('\n');
}
// Add the current line.
cueString.append(Util.truncateAscii(buildCurrentLine(), maxTextLength));

In the case of your The USS Cerritos remains in line, we have indent=16 and tabOffset=3, so we truncate the string to 32 - 16 - 3 = 13 characters.

The indent=16 comes from a packet with cc1=0x11 cc2=0x58 at timestamp 55488766µs. This part looks correct, Table 53 in the ANSI CEA-608-E 2014 spec shows that 0x58 indicates an indent of 16.

So I think your content is not strictly valid, because it tries to encode more than 32 characters into a single row (when accounting for the 16 char indent and the 3 char tab offset).

However, I think we can probably tweak our 'truncate to 32 chars' logic to ignore indentation and offset and only operate on the actual visible text characters. This will continue to resolve the concern in #7341 (screen full of chars) and will also mean your content works too. I'll send a change for this.

With this change it looks like this:

android-screenshot-2023-06-29T141153

@ZygmuntMalek
Copy link
Author

After your change our client text looks similar to version Exoplayer 2.11.8 - so IMHO looks good - is this change available at some dev branch ?

@icbaker
Copy link
Collaborator

icbaker commented Jun 30, 2023

I've submitted the change internally, it will be pushed to the dev-v2 branch here and the media3 main branch in the next week or so. The commits will be linked in this issue when it's pushed.

@icbaker icbaker closed this as completed Jun 30, 2023
microkatz pushed a commit to androidx/media that referenced this issue Jul 5, 2023
We introduced truncation to 32 chars in <unknown commit>
and included indent and offset in the calculation. I think this is
technically correct, but it causes problems with the content in
Issue: google/ExoPlayer#11019 and it doesn't seem a problem to only truncate actual
cue text (i.e. ignore offset and indent).

#minor-release

PiperOrigin-RevId: 544677965
microkatz pushed a commit that referenced this issue Jul 5, 2023
We introduced truncation to 32 chars in <unknown commit>
and included indent and offset in the calculation. I think this is
technically correct, but it causes problems with the content in
Issue: #11019 and it doesn't seem a problem to only truncate actual
cue text (i.e. ignore offset and indent).

#minor-release

PiperOrigin-RevId: 544677965
tianyif pushed a commit that referenced this issue Aug 11, 2023
We introduced truncation to 32 chars in <unknown commit>
and included indent and offset in the calculation. I think this is
technically correct, but it causes problems with the content in
Issue: #11019 and it doesn't seem a problem to only truncate actual
cue text (i.e. ignore offset and indent).

#minor-release

PiperOrigin-RevId: 544677965
(cherry picked from commit d5e4520)
tianyif pushed a commit to androidx/media that referenced this issue Aug 14, 2023
We introduced truncation to 32 chars in <unknown commit>
and included indent and offset in the calculation. I think this is
technically correct, but it causes problems with the content in
Issue: google/ExoPlayer#11019 and it doesn't seem a problem to only truncate actual
cue text (i.e. ignore offset and indent).

PiperOrigin-RevId: 544677965
(cherry picked from commit e8fdd83)
tianyif pushed a commit that referenced this issue Aug 14, 2023
We introduced truncation to 32 chars in <unknown commit>
and included indent and offset in the calculation. I think this is
technically correct, but it causes problems with the content in
Issue: #11019 and it doesn't seem a problem to only truncate actual
cue text (i.e. ignore offset and indent).

#minor-release

PiperOrigin-RevId: 544677965
(cherry picked from commit d5e4520)
@google google locked and limited conversation to collaborators Aug 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants