Cea608 - Check parity and valid bits #5086
Conversation
[Problem] Test streams and Over-the-Air content might feed corrupt data to the decoder [Solution] Check the parity bit of each byte. The validity bit of the header also needs to be checked and the screen needs to be immediately cleared for failing any of the new tests. [Test] - Live over-the-air content having 608 captions - The sample HLS streams having 608 caption channels - The Sarnoff file C40UDTAb testing parity failures.
| // But any over-the-air content and test streams might give the decoder corrupted data, so we | ||
| // need to drop those. Every data byte should have odd parity. | ||
| // When corrupt data is met the screen needs to be cleared, so we: | ||
| // - reset the cue builders |
There was a problem hiding this comment.
Please address comment on previous pull request about whether resetting is the best thing to do here.
There was a problem hiding this comment.
I agree, there are conflicting ideas in the legal documents about CEA608. This is from the Sarnoff compliance test document:
When the “NTSC bad parity test” is active, CC1, CC3 and CS1 should not display new captions. CC1and CC3 captions should persist on the screen for approximately one half second of this test condition, then the captions should clear.
In this case the test starts with correct data, then with bad parity after one half second, and they require the screen to be cleared.
There was a problem hiding this comment.
Checking the history of these changes suggest that we did not clear the screen in the first few iterations. And it was not leading previous captions staying on the screen too long. The original 608 documents suggested something like (this is my interpretation of the idea from the 80s)
- if the a command is failing the parity check: skip the command, wait for the repetition.
- if a repeated command is failing the parity check: decoder acted upon the first one.
- if the character fails the parity check, insert a placeholder character.
- if the stream is repeatedly failing the parity check, disable the decoding.
With the current over-the-air content and reliable transmission the only one still happening is the last case. The entire stream seems to have random data and we should not let the last caption stay on screen forever. Shall I add a TODO to add additional conditions later?
| if ((ccDataHeader & CC_VALID_FLAG) != CC_VALID_FLAG) { | ||
| // The screen needs to be cleared when the closed captions are turned off | ||
| // (the encoder flips the validity bit) | ||
| resetCueBuilders(); |
There was a problem hiding this comment.
Following on from the question of resetting when the valid flag is not set:
Isn't it a problem that above on L323, you do not reset if ccData1 == 0 && ccData2 == 0? If I were going to not set the valid flag, the most obvious thing I'd fill the two bytes with would be zeros. This will cause the caption to not be cleared, however you're saying that clearing is necessary.
The same applies to clearing if the parity is wrong. If that's what needs to happen, I think that decision would somehow need to be made before the ccData1 == 0 && ccData2 == 0 case.
There was a problem hiding this comment.
You arguments are logical, those are completely valid for on-demand content. I tested various order of the checks, and they fail in case of Over-The-Air content, the only use-case when CEA608 should be used.
We had dozens of iterations of ordering of these checks and this current one is the latest that works with real-life 608 streams. I cannot give real arguments listed in the specification, but most probably:
- The original content provider (who made the content 20 years ago) were the ones flipping the Validity bit, but they only created the content with one specific frame rate.
- Any later content distributor might change the frame rate. That requires adding extra bytes to the stream without modifying the timing, so the zero bytes pairs must be introduced - but they must not modify anything else, like clearing the screen. So the zero byte pairs simply must restart the decoding loop, and must be checked earlier than the validity flag. (Later changes not in this change set will introduce additional conditions to solve issues introduced by frame rates not existing when 608 was defined.)
- The stream corruption (not possible for on-demand content) is most probably introduced by the transmission, should be rare and it should not influence any of the additional states I will later introduce to these checks above. So corruption is checked last in this case.
Please run these changes with over-the-air content, and you will see it renders captions with much better accuracy than it was before. Unfortunately, I still have dozens of changes after these ones, and they might depend on the ordering of the checks introduced in this change, so the reason for this current ordering might not be clear at this point. It simply works. If it cases regressions, I might not aware of, or it does not handle some types of content I have not met yet, we can identify those as long as these changes are in the dev branch)
There was a problem hiding this comment.
This might be the hint for the requirement for clearing the screen when valid flag is not set
ANSI / CTA - 608 - ER - 2014 Section C.21:
Display Enable/Disable Logic and Timing (Regulatory)
...
Decoders also should automatically disable the display and erase memory in response to loss of valid data on line 21.
There was a problem hiding this comment.
Ok. Let's merge this with the order you propose. However note that we can't promise not to change it if we get reports that this is breaking streams being played by other developers / providers.
|
It would still be helpful to split these into separate pull requests. The changes seem independent to one another, in which case there's no reason to tie them into a single PR. |
You asked for separate commits (here), not necessarily separate pull requests. I will remove the last 2 commits. Update: DONE |
|
We're ready to merge this, but it needs to be into |
|
I've created #5140 to merge this, so you don't need to take any action. Sorry for the churn. We should be able to get subsequent changes landed with less hassle. To clarify:
Thanks! |
I start refactoring the previously rejected "one big change" into smaller fixes, optimisations, fine tuning etc.
Please add code review comments as early as possible to avoid complex rebasing and conflicts.
Code review comments from #5075 are addressed. I probably should have force pushed the changes instead of creating a new branch. Next time ;).