Cea608Decoder Span parameter Crash

[zsmatyas]

This line tends to crash quite often for live content:

ExoPlayer/library/core/src/main/java/com/google/android/exoplayer2/text/cea/Cea608Decoder.java

Line 708 in 2b55c91

captionStringBuilder.setSpan(cueStyle.style, cueStyle.start, end,

I had a pull request for handling the various styles in Cea608Decoders for ExoPlayer 1. For ExoPlayer 2, you have included many parts of that pull request, but you also re-designed the handling of the styles by adding the CueStyle class. So my understanding of the CueStyle might be different than yours.

This current code creates list of CueStyles, and adds them only when the captions are rendered. I could identify one case when this current solution crashes. Incoming caption bytes are:

MISC: RESUME_CAPTION_LOADING
MISC: ERASE_DISPLAYED_MEMORY
MISC: END_OF_CAPTION
MISC: RESUME_CAPTION_LOADING
PAC: Row:14; Col:1; Color:KEEP PREVIOUS COLOR; italic:true; underline:false
Incoming char: 'A'
Incoming char: 'l'
Incoming char: 'e'
Incoming char: 'v'
Incoming char: 'e'
Incoming char: ' '
Incoming char: 'P'
Incoming char: 'M'
Incoming char: ' '
Incoming char: 'f'
Incoming char: 'o'
Incoming char: 'r'
Incoming char: ' '
Incoming char: 'a'
Incoming char: ' '
Incoming char: 'B'
Incoming char: 'e'
Incoming char: 't'
Incoming char: 't'
Incoming char: 'e'
Incoming char: 'r'
Incoming char: ' '
Incoming char: 'A'
Incoming char: 'M'
Incoming char: '.'
MISC: RESUME_CAPTION_LOADING
MISC: ERASE_DISPLAYED_MEMORY
MISC: END_OF_CAPTION
MISC: ERASE_DISPLAYED_MEMORY
MISC: RESUME_DIRECT_CAPTIONING
PAC: Row:15; Col:1; Color:WHITE; italic:false; underline:false
MRC: ITALIC TURNED ON
Incoming char: ' '
MISC: BACKSPACE

The crash itself is:

05-26 08:49:36.592 E/ExoPlayerImplInternal(13783): Internal runtime error.
05-26 08:49:36.592 E/ExoPlayerImplInternal(13783): java.lang.IndexOutOfBoundsException: setSpan (1 ... 0) has end before start
05-26 08:49:36.592 E/ExoPlayerImplInternal(13783): 	at android.text.SpannableStringBuilder.checkRange(SpannableStringBuilder.java:1313)
05-26 08:49:36.592 E/ExoPlayerImplInternal(13783): 	at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:683)
05-26 08:49:36.592 E/ExoPlayerImplInternal(13783): 	at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:676)
05-26 08:49:36.592 E/ExoPlayerImplInternal(13783): 	at com.google.android.exoplayer2.text.cea.Cea608Decoder$CueBuilder.buildSpannableString(Cea608Decoder.java:1190)

(As the line numbers suggest, we have some modifications in Cea608Decoder that is not shared (yet), but the crash should be present irrespective of our changes.)

The issue seems to be the independent handling of the characters and the spans: backspace should be able to clear any characters and possibly create empty spans as well. This list of Spans applied right before rendering seems to be a fragile solution. In this specific case leading to the crash, turning on ITALIC adds two items to the span list:

ExoPlayer/library/core/src/main/java/com/google/android/exoplayer2/text/cea/Cea608Decoder.java

Lines 384 to 385 in 2b20780

	currentCueBuilder.setMidrowStyle(new StyleSpan(Typeface.ITALIC), 2);
	currentCueBuilder.setMidrowStyle(new ForegroundColorSpan(Color.WHITE), 1);

Then comes a BackSpace command and our indices become invalid.

The crash can be avoided simply be checking the validity of the Span indices for the current content of the StringBuilder. Although it is hard to prove that the intention of the caption provider was matching the rendered result. I have a few question:

1. Is the CueStyle.nextStyleIncrement member introduced only to handle the priorities of the CEA608 styles?
2. Shouldn’t the preamble and mid-row styles handled in a single list as they both contain the same styling options?
3. As I recall, my version of Span-handling added them whenever a span is closed. For example, when you set a color, all previous color spans should be closed instead of adding cascading color spans to the string. Isn't that a better approach?
4. Currently, the Italic command adds a Span for Italic, and also a color White Span. Shouldn’t we replace the "adding the white span" with "closing any previous color spans" instead? Adding a white span means that this might rendered in white irrespective of the "default color" setting of the user. (Cea608 had implicit white as default color and was not prepared to have any user settings like the ones provided by CaptioningManager)

[linhai326]

we saw the same issue and we had to add length checks in all setSpan() calls in cea608Decoder.java and cea708decoder.java

[ojw28]

Is the CueStyle.nextStyleIncrement member introduced only to handle the priorities of the CEA608 styles?

The person who wrote this code no longer works on the project, however looking at the corresponding code review, it appears the intention is to handle the white-italic case, where two spans are used to apply a single style:

https://github.com/google/ExoPlayer/blob/dev-v2/library/core/src/main/java/com/google/android/exoplayer2/text/cea/Cea608Decoder.java#L383

The nextStyleIncrement ensures both of the corresponding spans are ended at the correct position (ignoring other bugs that might make this statement untrue ;)).

Shouldn’t the preamble and mid-row styles handled in a single list as they both contain the same styling options?

I don't know enough about the spec to answer this, except to say that the comments in the code suggest not. Are you seeing visible issues that would be resolved by doing this?

As I recall, my version of Span-handling added them whenever a span is closed. For example, when you set a color, all previous color spans should be closed instead of adding cascading color spans to the string. Isn't that a better approach?

Are you sure cascading spans are added? It looks to me like spans have their end points calculated when they're added:

https://github.com/google/ExoPlayer/blob/dev-v2/library/core/src/main/java/com/google/android/exoplayer2/text/cea/Cea608Decoder.java#L705

Which seems like pretty much the same thing as you're describing... Although perhaps it's a bug that buildSpannableString is adding spans directly to captionStringBuilder, rather than to a local variable. The way the code is structured currently, could multiple calls to buildSpannableString without captionStringBuilder being cleared in-between cause duplicate spans to be added? I doubt it would do any harm, but presumably it's not very efficient.

Currently, the Italic command adds a Span for Italic, and also a color White Span. Shouldn’t we replace the "adding the white span" with "closing any previous color spans" instead? Adding a white span means that this might rendered in white irrespective of the "default color" setting of the user. (Cea608 had implicit white as default color and was not prepared to have any user settings like the ones provided by CaptioningManager)

I think so, yes.

In summary, I think there are a few issues here:

1. A possible bug that buildSpannableString adds spans directly to captionStringBuilder, rather than to a local variable.
2. Not correctly handling backspace in conjunction with spans.
3. Using white rather than clearing the color for the italic command.

Does that seem plausible to you? I can open a pull request with some proposed fixes (although I probably wont be able to test them that thoroughly; that would be your job ;)).

[ojw28]

Do you have any good test content for demonstrating these issues?

[zsmatyas]

Most crashes were identified by just leaving a local live channel play overnight (any channel that should also be available in the Mountain View area).
I added checks for all setSpan() calls to check the indices for validity. I haven't seen any more crashes since then.

I usually test the code including all these changes as well: #4308 (and much more, but the caption features can be usually separated from other changes). If the change is more-or-less merge-able onto the head of the pull request, I can probably test them.

[ojw28]

Currently, the Italic command adds a Span for Italic, and also a color White Span. Shouldn’t we replace the "adding the white span" with "closing any previous color spans" instead? Adding a white span means that this might rendered in white irrespective of the "default color" setting of the user. (Cea608 had implicit white as default color and was not prepared to have any user settings like the ones provided by CaptioningManager)

A further thought about this: It seems problematic that, if we were to do this, there's a way to return to "default color" whilst also changing to italic, but not otherwise (i.e. non-italic). It kind of suggests we should treat WHITE as "default color" too, although that is in itself slightly strange. Alternatively, we could make all 608 captions explicitly white. Hmm!

[zsmatyas]

Are you sure cascading spans are added?

You are right, the mid-row spans are using this nextStyleIncrement values I do not fully get yet. But as I see, at least the midrow spans are inside the preamble spans, aren't they?

About Default color: My original Pull request for ExoV1 handled white as "default color" everywhere. So instead of opening a new color span for white, I simply closed the previous color span. The ExoV2 caption handling shows lots of similarities, but this part was not kept:
https://github.com/google/ExoPlayer/pull/1312/files#diff-e9fd670327a94df1ab96714b095f4985R416
It is not a serious difference, I have not found (legal) requirement to pick any of two solutions.

About the nextStyleIncrement the 608 standard has priorities for the formatting: https://www.law.cornell.edu/cfr/text/47/79.101

(ii) The color attribute has the highest priority and can only be changed by the Mid-Row Code of another color. Italics has the next highest priority. If characters with both color and italics are desired, the italics Mid-Row Code must follow the color assignment. Any color Mid-Row Code will turn off italics. If the least significant bit of a Preamble Address Code or of a color or italics Mid-Row Code is a 1 (high), underlining is turned on. If that bit is a 0 (low), underlining is off.

So I implemented it by closing previous spans in order of the priorities, so there was no way to create an invalid order. I did not use 2 list of spans, one for collecting the preamble and one for the mid-row styling. My version used the theoretical limitation that there can be only 3 spans open at any time: one color, one underline and one italic. The current HEAD seems to have a list for MidRowStyle-s, and whenever we add a new style (start a new span) we also save new nextStyleIncrement values so we know which "next start span opening position" to use to close this current span?

ExoPlayer/library/core/src/main/java/com/google/android/exoplayer2/text/cea/Cea608Decoder.java

Line 706 in 2b20780

? midrowStyles.get(i + cueStyle.nextStyleIncrement).start

Might be correct, but hard to interpret that it works as intended.

[ojw28]

This should be fixed by the change above, although we've yet to acquire test streams that allow us to thoroughly test the change. Please verify.

[baconz]

@ojw28 I'm still able to reproduce this on 2.8.2.

Here's a simple 3-segment manifest that will repro:

https://static-us-east-2-fastly-a.www.philo.com/storage/cea608-exo-bug/manifest.mpd

My naive solution would be to check that end > cueStyle.start before the setSpan, but maybe somebody with deeper experience in the 608 code can come up with a correct solution.

[ojw28]

@baconz - The fix hasn't made it into a release yet, so that's expected. Thanks for the sample, that's really helpful. I can confirm that the failure does not occur with this sample in our dev-v2 branch.

[baconz]

@ojw28 sorry, I missed that! I saw mentions of CEA-608 improvements in the 2.8.2, and didn't bother to check if they were these improvements.

Let me know if I can further assist in testing, or if you have any questions about the content I posted.