CEA608: Limiting duplicated command checks to immediate frames #5355

zsmatyas · 2019-01-09T20:33:03Z

Reported in #3860
For failing examples see the github link above.

[Problem]
We drop matching control codes even if they are not received on
consecutive frames.

The specification says
"(4) If the first transmission of a control code pair passes parity,
it is acted upon within one video frame. If the NEXT frame contains
a perfect repeat of the same pair, the redundant code is ignored."

Keyword is the NEXT. The frames must arrive immediately after
each other.

See https://www.law.cornell.edu/cfr/text/47/79.101

[Solution]
Set an additional flag when any data is processed. Control code
duplication checks should be limited only for the first control
byte pairs processed after any control code.

[Test]
Sarnoff tests have equivalent CEA708 and CEA608 Streams.

zsmatyas · 2019-01-09T20:37:20Z

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

        }
        continue;
      }

-      // If we've reached this point then there is data to process; flag that work has been done.
-      captionDataProcessed = true;


I moved this line above, as I think it should be set for any invalid caption as well. That will cause the screen update, I do not see a reason why we do not want to update the screen for invalid captions.

What does doing this fix? captionDataProcessed is already set to true on L332 if there's a valid->invalid transition. For invalid->invalid the screen will already be blank, so updating the screen wont do anything useful?

I just wanted to reduce redundancy, instead of setting the flag inside and after this if block, we can achieve the same by setting it before the block, can't we?

I think the visual result is the same, but in the case that the encoder has turned captions off by flipping the validity bit you're causing a whole load of unnecessary work to happen on every single frame.

ojw28 · 2019-01-09T22:19:45Z

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

        }
        continue;
      }

-      // If we've reached this point then there is data to process; flag that work has been done.
-      captionDataProcessed = true;


What does doing this fix? captionDataProcessed is already set to true on L332 if there's a valid->invalid transition. For invalid->invalid the screen will already be blank, so updating the screen wont do anything useful?

ojw28 · 2019-01-09T22:28:17Z

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

@@ -230,6 +230,7 @@
  private boolean repeatableControlSet;
  private byte repeatableControlCc1;
  private byte repeatableControlCc2;
+  private boolean dataProcessedSinceLastRepeatableControl = true;


Why does this default to true? Ditto for flush. We haven't seen any data at all in these cases, so I don't think it makes sense for it to be true. Comment may be obsolete if you can get rid of the field as per comment below.

The very first "repeatable control" should not be handled as a repetition: default = true.

ojw28 · 2019-01-09T22:41:47Z

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

        }
        continue;
      }

-      // If we've reached this point then there is data to process; flag that work has been done.
-      captionDataProcessed = true;
+      if (dataProcessedSinceLastRepeatableControl) {


Do you understand what this bit of existing code is doing (which is kept in this change; I'm posting this comment here because GitHub doesn't let me comment on the line I want to comment on):

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

Line 329 in 71f72c5

if (!isRepeatableControl) {

I can't figure out what it's supposed to be doing. Perhaps it was an attempt to implement what you're now doing properly, and so becomes unnecessary?

Either way, does it work to get rid of both that bit of code and dataProcessedSinceLastRepeatableControl, and just do this here:

boolean repeatedControlPossible = repeatableControlSet; repeatableControlSet = false;

and then pass repeatedControlPossible as an argument to handleCtrl? The line that sets repeatableControlSet = false inside handleCtrl would also go away.

repeatableControlSet seems to only save the value between 2 iterations of the loop.

This part is probably only disabling the check for duplications between two calls of decode():

if (!isRepeatableControl) { repeatableControlSet = false; }

I do not see why that is necessary at the moment. If 608 bytes are the first ones in each packet (as 608 bytes have fixed bandwidth), then 708 bytes are next to each other, then 2 calls to decode() probably cannot contain originally consecutive bytes in the analogue CEA stream.

Your proposal seems logical, I do not seem any drawbacks of that solution, let me update the code.

(Unfortunately, I cannot run full test cycles including various live content, sarnoff test cases etc. for every code change separately, but I will run them as soon as we have more - maybe all changes merged.)

ojw28 · 2019-01-10T10:56:08Z

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

-      // If we've reached this point then there is data to process; flag that work has been done.
-      captionDataProcessed = true;
+      boolean repeatedControlPossible = repeatableControlSet;
+      repeatableControlSet = false;


Should these lines go above the block that handles invalid captions? I doubt it's correct to de-duplicate repeated commands that have invalid data in between. Not sure though :).

I think I had these checks a few line above this current position before and it failed some live streams. My theory was that the original content was made for 30FPS, and when they insert extra frames to be able to play it in 60FPS, they insert zero byte pairs for the new empty frames. Zero check is a little bit earlier, I do not think it would make a difference having this before or after the validity check if it is always after the zero-byte-pair check.

I was thinking it should go after the zero check and before the validity check. In case captions are toggled off by flipping the validity bit after a repeatable command, and then when captions are toggled on again the first piece of data happens to be the same repeatable command. Seems like an edge case, but may as well handle it given it's just moving the lines up a little?

I will move this up as part of the merge.

ojw28 · 2019-01-10T10:57:06Z

Two pending comments (the one above, and the one about putting captionDataProcessed back). Otherwise good to merge; thanks!

Reported in google#3860 For failing examples see the github link above. [Problem] We drop matching control codes even if they are not received on consecutive frames. The specification says "(4) If the first transmission of a control code pair passes parity, it is acted upon within one video frame. If the NEXT frame contains a perfect repeat of the same pair, the redundant code is ignored." Keyword is the NEXT. The frames must arrive immediately after each other. See https://www.law.cornell.edu/cfr/text/47/79.101 [Solution] Set an additional flag when any data is processed. Control code duplication checks should be limited only for the first control byte pairs processed after any control code. [Test] Sarnoff tests have equivalent CEA708 and CEA608 Streams.

zsmatyas · 2019-01-10T20:37:12Z

Have I addressed all comments? It is hard to follow after I force pushed changes.

ojw28 · 2019-01-10T23:37:50Z

Think so yes; thanks!

PiperOrigin-RevId: 229364147

googlebot added the cla: yes label Jan 9, 2019

zsmatyas commented Jan 9, 2019

View reviewed changes

zsmatyas mentioned this pull request Jan 9, 2019

CEA608 changes to pass Sarnoff tests #4900

Closed

ojw28 reviewed Jan 9, 2019

View reviewed changes

ojw28 self-assigned this Jan 9, 2019

zsmatyas force-pushed the dev-v2 branch from efbd782 to 89d932e Compare January 10, 2019 00:06

ojw28 reviewed Jan 10, 2019

View reviewed changes

zsmatyas force-pushed the dev-v2 branch from 89d932e to fff6023 Compare January 10, 2019 20:18

ojw28 added the should merge label Jan 10, 2019

ojw28 merged commit fff6023 into google:dev-v2 Jan 15, 2019

ojw28 added a commit that referenced this pull request Jan 15, 2019

Merge pull request #5355 from zsmatyas:dev-v2

3b5e8ad

PiperOrigin-RevId: 229364147

zsmatyas deleted the dev-v2 branch January 15, 2019 21:34

google locked and limited conversation to collaborators May 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CEA608: Limiting duplicated command checks to immediate frames #5355

CEA608: Limiting duplicated command checks to immediate frames #5355

zsmatyas commented Jan 9, 2019

zsmatyas Jan 9, 2019 •

edited

Loading

ojw28 Jan 9, 2019

zsmatyas Jan 9, 2019 •

edited

Loading

ojw28 Jan 10, 2019

zsmatyas Jan 10, 2019

ojw28 Jan 9, 2019

ojw28 Jan 9, 2019

zsmatyas Jan 9, 2019

ojw28 Jan 9, 2019

zsmatyas Jan 9, 2019

ojw28 Jan 10, 2019

zsmatyas Jan 10, 2019

ojw28 Jan 10, 2019

ojw28 Jan 14, 2019

ojw28 commented Jan 10, 2019

zsmatyas commented Jan 10, 2019

ojw28 commented Jan 10, 2019

CEA608: Limiting duplicated command checks to immediate frames #5355

CEA608: Limiting duplicated command checks to immediate frames #5355

Conversation

zsmatyas commented Jan 9, 2019

zsmatyas Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsmatyas Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojw28 commented Jan 10, 2019

zsmatyas commented Jan 10, 2019

ojw28 commented Jan 10, 2019

zsmatyas Jan 9, 2019 •

edited

Loading

zsmatyas Jan 9, 2019 •

edited

Loading