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

CEA608 changes to pass Sarnoff tests #4900

Closed
wants to merge 1 commit into from

Conversation

peddisri
Copy link
Contributor

@peddisri peddisri commented Oct 3, 2018

  1. NTSC parity and validity bit check added
  2. process_cc_data_flag check added
  3. PAINT ON mode support added
  4. Fixed line alignment issues for multi-line text
  5. Fixed issue of dropping too many duplicate control commands
  6. Fix for showing CC1 even if other channels are selected
  7. Filtering out TEXT and XDS content
  8. Fix for missing whitespaces when italic or other mid-row formatting
    is used
  9. Fix for NTSC Deleted tests of sarnoff
  10. Log illegal control codes instead of throwing exception

1. NTSC parity and validity bit check added
2. process_cc_data_flag check added
3. PAINT ON mode support added
4. Fixed line alignment issues for multi-line text
5. Fixed issue of dropping too many duplicate control commands
6. Fix for showing CC1 even if other channels are selected
7. Filtering out TEXT and XDS content
8. Fix for missing whitespaces when italic or other mid-row formatting
is used
9. Fix for NTSC Deleted tests of sarnoff
10. Log illegal control codes instead of throwing exception
@ojw28
Copy link
Contributor

ojw28 commented Oct 3, 2018

As noted on previous pull requests, if you can describe the change in the form of 10 bullet points, it should really be split into 10 pull requests, or at least 10 commits within a single pull request so the changes can be looked at in isolation. It's really hard to review changes where everything is just squashed together like this.

@peddisri
Copy link
Contributor Author

peddisri commented Oct 5, 2018

okie :-)

@zsmatyas
Copy link
Contributor

zsmatyas commented Nov 8, 2018

Splitting this change into smaller change sets:
#5075

@zsmatyas
Copy link
Contributor

Second pull request:
#5147 (fixes for paint-on mode)

@ojw28
Copy link
Contributor

ojw28 commented Jan 9, 2019

Is this an accurate reflection of the current state for merging these changes:

  1. NTSC parity and validity bit check added merged
  2. process_cc_data_flag check added
  3. PAINT ON mode support added merged
  4. Fixed line alignment issues for multi-line text merged
  5. Fixed issue of dropping too many duplicate control commands
  6. Fix for showing CC1 even if other channels are selected
  7. Filtering out TEXT and XDS content
  8. Fix for missing whitespaces when italic or other mid-row formatting is used probably fixed via afc19bf, so no longer needed?
  9. Fix for NTSC Deleted tests of sarnoff
  10. Log illegal control codes instead of throwing exception

(2) and (6) sound relatively trivial to get merged next (although I didn't look at which code changes those correspond to). Possibly (7) and (10) as well?

@zsmatyas
Copy link
Contributor

zsmatyas commented Jan 9, 2019

Yes, that state seems to be correct. I have a list of all commits with comments what changes can be squashed in what order, so the next ones on my list are:
5. Fixed issue of dropping too many duplicate control commands
6. Fix for showing CC1 even if other channels are selected
7. Filtering out TEXT and XDS content + 10. squashed into it as they are the same topic

  1. Is already fixed with the commit you linked.

  2. is implemented in CeaUtil.java, so the order of the commits is not important.

@zsmatyas
Copy link
Contributor

zsmatyas commented Jan 9, 2019

Last merged Line alignment fix is #5201

Now added fix for duplicated control code checks:
#5355
This should fix #3860

@zsmatyas
Copy link
Contributor

Handling channel bit: #5390

@ojw28
Copy link
Contributor

ojw28 commented May 5, 2019

Is it just (9) and (10) from the original list at the top of this pull request that are still outstanding? Are we close enough that this pull request can be closed? Thanks!

@ojw28
Copy link
Contributor

ojw28 commented May 20, 2019

Closing this since most of it has landed elsewhere. If there are pending changes (e.g. for (9) and (10)), I would assume they will come as separate pull requests). Thanks for your help!

@ojw28 ojw28 closed this May 20, 2019
@google google locked and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants