Skip to content

CEA-708: Update current row value when new line is added#1315

Merged
copybara-service[bot] merged 6 commits intoandroidx:mainfrom
dathdoan:ClosedCaption_708Decoder_bugfix
May 23, 2024
Merged

CEA-708: Update current row value when new line is added#1315
copybara-service[bot] merged 6 commits intoandroidx:mainfrom
dathdoan:ClosedCaption_708Decoder_bugfix

Conversation

@dathdoan
Copy link
Copy Markdown
Contributor

@dathdoan dathdoan commented Apr 25, 2024 •
edited by icbaker
Loading

Update current row value when new line is added. This is to prevent an incorrect newline to be added in setPenLocation.
@icbaker
Copy link
Copy Markdown
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'?
@dathdoan
Copy link
Copy Markdown
Contributor Author

dathdoan commented May 21, 2024

Thanks, I've updated the PR title and the code changes to reflect the fix for fix 2 only.
@icbaker
Copy link
Copy Markdown
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. May 21, 2024
copybara-service bot pushed a commit that referenced this pull request May 21, 2024
@icbaker @copybara-github
Re-apply CEA-608 validDataChannelTimeoutMs assertion
8fdf105 
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
Copy Markdown
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?
@dathdoan
Copy link
Copy Markdown
Contributor Author

dathdoan commented May 22, 2024

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 icbaker force-pushed the ClosedCaption_708Decoder_bugfix branch from 2f96dc0 to 8eda764 Compare May 23, 2024 14:24
@icbaker
Copy link
Copy Markdown
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
@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.

Labels

should merge

2 participants

@dathdoan @icbaker