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-708: Update current row value when new line is added #1315

Merged

Conversation

datdoantelus
Copy link
Contributor

@datdoantelus datdoantelus commented Apr 25, 2024

Update current row value when new line is added. This is to prevent an incorrect newline to be added in setPenLocation.

@icbaker
Copy link
Collaborator

icbaker commented May 21, 2024

Ah it looks like we previously merged the rowLock/columnLock fix (from you) in #942, and then it got accidentally reverted (by me) in dd2e4a5. I'm really sorry about that. As this part is more like a fix on top of dd2e4a5, rather than an 'original' change, I'm going to make this fix internally rather than directly merge this PR - the end result should be the same, but it will be a bit easier for me to tell the story of how this change ended up getting lost (for future code archaeologists). Hope that's OK!

I think 'Fix 2' in this PR wasn't part of #942 - so maybe you could update this PR to only contain 'Fix 2'?

@datdoantelus
Copy link
Contributor Author

Thanks, I've updated the PR title and the code changes to reflect the fix for fix 2 only.

@icbaker
Copy link
Collaborator

icbaker commented May 21, 2024

I've re-applied the rowLock/columnLock fix in e2847b3

@icbaker icbaker changed the title Ignore rowLock and numLock as define in CTA-708 spec. CEA-708: Update current row value when new line is added May 21, 2024
copybara-service bot pushed a commit that referenced this pull request May 21, 2024
This change was originally made in 379cb3b.

It was then accidentally lost in when `Cea608Parser` was merged back
into `Cea608Decoder` in 25498b1.

This was spotted when re-doing a similar lost change to `Cea708Decoder`,
reported in #1315.

See reasoning on e2847b3
about why this is the only 'lost' CEA-608 change.

PiperOrigin-RevId: 635803536
@icbaker
Copy link
Collaborator

icbaker commented May 22, 2024

This is to prevent an incorrect newline to be added in setPenLocation.

Would you be able to provide a bit more info about this? Even better if you can craft a test case that demonstrates the issue that is fixed by this change.

In particular I'm trying to understand how this interacts with the existing this.row = row assignment in setPenLocation() - I wonder if this assignment should be removed as part of this change?

@datdoantelus
Copy link
Contributor Author

I've added a couple of test related to newline handling and setPenLocation. Essentially, without the fix, we get an extra newline in the output.

@icbaker
Copy link
Collaborator

icbaker commented May 23, 2024

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@icbaker icbaker force-pushed the ClosedCaption_708Decoder_bugfix branch from 8eda764 to e87b820 Compare May 23, 2024 14:27
@copybara-service copybara-service bot merged commit 0a58832 into androidx:main May 23, 2024
1 check passed
@androidx androidx locked and limited conversation to collaborators Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants