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 caption goes out of safe area (bleeds out of screen) in Roll-up mode #7475

Closed
sneelavara opened this issue Jun 7, 2020 · 9 comments
Assignees
Labels

Comments

@sneelavara
Copy link
Contributor

Issue description

Summary 

CEA-608 caption goes out of safe area (bleeds out of screen) in Roll-up mode

Root Cause

On certain Live TV channels the Caption from Live TV program bleeds out of the entire screen.  This happens in Roll-Up mode and when the caption start at the left top with row number less than or equals to 2.

The Cea608Decoder.java line number 890

if (captionMode == CC_MODE_ROLL_UP || row > (BASE_ROW / 2)) {

has this logic -

if (captionMode == CC_MODE_ROLL_UP || row > (BASE_ROW / 2)) {
  lineAnchor = Cue.ANCHOR_TYPE_END;
  line = row - BASE_ROW;
  // Two line adjustments. The first is because line indices from the bottom of the window
  // start from -1 rather than 0. The second is a blank row to act as the safe area.
  line -= 2;
} else {
  lineAnchor = Cue.ANCHOR_TYPE_START;
  // Line indices from the top of the window start from 0, but we want a blank row to act as
  // the safe area. As a result no adjustment is necessary.
  line = row;
}

Roll-up caption with row number on top portion of the screen are also pulled up by two lines because of this code. When the row number is less than 2, the Roll-up caption is drawn on the top of the screen. I think this violates the safe area restriction. The effect of this is the caption from the first row is cut off.

Changing the if condition at line 890 to -

if (row > (BASE_ROW / 2)

i.e. excluding Roll-up caption mode check, fixes the caption bleeding issue. Line 890 seem to be modified as part of commit (e86bfd6).

Could you please explain why the Roll-up captions are always pulled up by 2 lines? What could be the side effect of not checking for Roll-up mode at line number 890?

Reproduction steps

Using the sample HLS stream, play to the stream. Observe the caption when it’s on top left screen.

Link to test content

Content is broadcast from cable, so link was sent to exoplayer dev email

A full bug report captured from the device

N/A

Version of ExoPlayer being used

2.11.4 

Device(s) and version(s) of Android being used

Tested and reproduced on Arris and Amino AndroidTV devices running Android Pie.  Reproduction is 100%.

@icbaker
Copy link
Collaborator

icbaker commented Jun 8, 2020

That if block is trying to deal with lines it thinks should be in the bottom half of the screen. I agree roll-up cues shouldn't be assumed to be in the bottom half of the screen though - I've sent a change that removes captionMode == CC_MODE_ROLL_UP from the condition.

icbaker added a commit that referenced this issue Jun 8, 2020
ANSI/CTA-608-E R-2014 Annex B.5 says:
"The concept of a movable base row for a roll-up caption is new."

It means "new" compared to TC1 and TC2 (released in or before 1985).

Issue: #7475
PiperOrigin-RevId: 315258859
@sneelavara
Copy link
Contributor Author

Thank you very much @icbaker. That would greatly help us addressing the regulatory defect.

@icbaker
Copy link
Collaborator

icbaker commented Jun 8, 2020

This is now fixed on dev-v2.

@icbaker icbaker closed this as completed Jun 8, 2020
@zsmatyas
Copy link
Contributor

zsmatyas commented Jun 8, 2020

We implemented this a long time ago, so please forgive me if my recollection is incorrect.

@sneelavara Could you also provide the last few instructions leading to the issue? You should not have more than 2 lines rolling if you are not at least in the second row.

  if (captionMode == CC_MODE_ROLL_UP || row > (BASE_ROW / 2)) {

Has the Roll-up mode checked separately, as roll up captions always grow upwards, their location is based on the last line of the Roll-up captions so they need to use ANCHOR_TYPE_END.

In exoPlayer, as I recall, the lines on the screen are numbered as:


1
2
3
4
...
-4
-3
-2
-1

Row -1 should not be used (unless there are other adjustments for the safe area), and the second adjustment seems to be needed for:

https://github.com/google/ExoPlayer/blob/release-v2/library/ui/src/main/java/com/google/android/exoplayer2/ui/SubtitlePainter.java#L385

seems to add one to the index to calculate the position from the next row.

@icbaker icbaker reopened this Jun 8, 2020
@icbaker
Copy link
Collaborator

icbaker commented Jun 8, 2020

Ah thanks for the clarification @zsmatyas, I didn't realise this bit:

Has the Roll-up mode checked separately, as roll up captions always grow upwards, their location is based on the last line of the Roll-up captions so they need to use ANCHOR_TYPE_END.

Your numbering of lines is semi-correct, but it's important to also consider that both of those numbering schemes extend past each other (positive numbers are just offset from the top of the screen, negative numbers from the bottom):

0    -7
1    -6
2    -5
3    -4
4    -3
5    -2
6    -1

Relying on negative line offsets (from the bottom) to position something near the top of the screen is risky - because you don't know how many lines will "fit" on the screen, so it's easy to overshoot (or end up with captions in the middle of the screen). It all depends on the font size the subtitles end being rendered in (which is, in general, user-configurable (though I guess you can disable that in your own apps)).

I think we should special-case CC_MODE_ROLL_UP in the 'top half of the screen' if-block to count the number of lines that have been rolled up (instead of using ANCHOR_TYPE_END). I'll experiment with that change tomorrow.

@zsmatyas
Copy link
Contributor

zsmatyas commented Jun 8, 2020

Could we make sure that the content we want to fix is according to the standard? Are the indices / values correct and rendered with the expected font sizes? What is the base row and how many rows are rolling?


As there are small adjustments for the exact positioning at many layers of the caption decoding / rendering, I do not think it is currently easy to use the top anchor, for example, having 3 lines of rolling captions, then switch to 2 lines by keeping the bottom 2 rows and keep them perfectly at the previous location of them.
This might be already complicated because of the font sizes affecting positioning. So in case we render the 2-4 lines based on a single anchor and it needs to change the row-count dynamically, we might need to rely on ANCHOR_TYPE_END to avoid moving the captions around.

Also note:
The default font sizes of exoplayer used to not match to CEA608 standard. Can you render 15 rows inside the safe area?

@icbaker
Copy link
Collaborator

icbaker commented Jun 8, 2020

I think it's risky to use negative line offsets to display content in the top half of the screen - the 15 CEA-608 lines are forgotten by the time ExoPlayer comes to render the cues and if it's trying to render something on line (2 - 15 - 2) = -15 (old method), that could well be off the top of the screen.

I did the experiment of using the rolled up lines count. In this case, row=2. This is what I found:

Without 770df86 (i.e. original behaviour, line=-15, anchor=end):

With 770df86 (current behaviour, line=2,anchor=start):

Adjusting to consider the number of rolled up lines (line=1,anchor=start):

@sneelavara
Copy link
Contributor Author

Thank you very much @icbaker and @zsmatyas for looking into this.
For Live broadcast its regulatory requirement to support user overrides. So, we can not block user from changing font size in our app. I have not tried/verified 15 rows within safe area. Will try that test.

@zsmatyas I don't think the stream violates the spec. It has RU2(0x14 0x25) follows by CR(0x14 0x2D) and PAC(0x11 0x70). Here is the CC byte pairs for the above two lines -

2020-06-08 17:05:16.542 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x14 cc2: 0x25
2020-06-08 17:05:16.602 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x14 cc2: 0x2D
2020-06-08 17:05:16.672 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x11 cc2: 0x70
2020-06-08 17:05:16.743 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x50 cc2: 0x4F
2020-06-08 17:05:16.774 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x49 cc2: 0x4E
2020-06-08 17:05:16.803 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x54 cc2: 0x20
2020-06-08 17:05:16.844 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x56 cc2: 0x41
2020-06-08 17:05:16.875 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x4C cc2: 0x55
2020-06-08 17:05:16.902 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x45 cc2: 0x53
2020-06-08 17:05:16.944 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x20 cc2: 0x41
2020-06-08 17:05:16.974 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x52 cc2: 0x45
2020-06-08 17:05:17.009 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x20 cc2: 0x54
2020-06-08 17:05:17.035 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x52 cc2: 0x49
2020-06-08 17:05:17.077 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x50 cc2: 0x4C
2020-06-08 17:05:17.108 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x45 cc2: 0x44
2020-06-08 17:05:17.142 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x2E cc2: 0x0

2020-06-08 17:05:19.170 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x14 cc2: 0x25
2020-06-08 17:05:19.241 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x14 cc2: 0x2D
2020-06-08 17:05:19.315 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x11 cc2: 0x70
2020-06-08 17:05:19.375 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x54 cc2: 0x4F
2020-06-08 17:05:19.413 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x50 cc2: 0x20
2020-06-08 17:05:19.446 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x34 cc2: 0x20
2020-06-08 17:05:19.474 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x41 cc2: 0x4E
2020-06-08 17:05:19.514 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x53 cc2: 0x57
2020-06-08 17:05:19.551 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x45 cc2: 0x52
2020-06-08 17:05:19.576 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x53 cc2: 0x20
2020-06-08 17:05:19.609 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x4F cc2: 0x4E
2020-06-08 17:05:19.648 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x20 cc2: 0x54
2020-06-08 17:05:19.678 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x48 cc2: 0x45
2020-06-08 17:05:19.708 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x20 cc2: 0x42
2020-06-08 17:05:19.749 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x4F cc2: 0x41
2020-06-08 17:05:19.779 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x52 cc2: 0x44
2020-06-08 17:05:19.810 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x2E cc2: 0x0
2020-06-08 17:05:22.141 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x14 cc2: 0x25
2020-06-08 17:05:22.214 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x14 cc2: 0x2D
2020-06-08 17:05:22.285 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x11 cc2: 0x70
2020-06-08 17:05:22.346 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x48 cc2: 0x45
2020-06-08 17:05:22.374 5422-5531/com.tivo.exoplayer.demo D/Cea608Decoder: decode: cc1: 0x52 cc2: 0x45

The first line "POINT VALUES ARE TRIPPLED" for us is cut half with this condition -

if (captionMode == CC_MODE_ROLL_UP || row > (BASE_ROW / 2)) {

icbaker added a commit that referenced this issue Jun 9, 2020
Reasoning and screenshots here:
#7475 (comment)

Issue: #7475
PiperOrigin-RevId: 315475888
@sneelavara
Copy link
Contributor Author

Thank you @icbaker, I verified commit 947485e, everything seem to work fine for us. Thank you.

@icbaker icbaker closed this as completed Jun 9, 2020
@google google locked and limited conversation to collaborators Aug 9, 2020
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

3 participants