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.

Sign up for GitHub

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

Jump to bottom

Fix IDR in two slices au split #27

Merged
merged 2 commits into from
Jun 19, 2023
Merged

Fix IDR in two slices au split #27

merged 2 commits into from
Jun 19, 2023

Conversation

bblaszkow06
Copy link
Member

  • AUSplitter test was missing flushing
  • I've replaced the enormous cond with private guards (they're using the fact that in case of a field missing, the guard won't crash but simply fail)
  • Removed faulty type == :idr clause
  • Added missing 2 clauses from the spec

Please re-check if all the conditions from the spec have been covered

varsill
varsill reviewed Jun 15, 2023
View reviewed changes
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with the simplification of the is_new_primary_coded_vcl_nalu , now it's much more clear. I only have some concerns about the default values for the fields, which might affect the conditions in is_new_primary_coded_vcl_nalu

lib/membrane_h264_plugin/parser/au_splitter.ex Show resolved Hide resolved
lib/membrane_h264_plugin/parser/au_splitter.ex Show resolved Hide resolved
@bblaszkow06 bblaszkow06 requested a review from varsill June 19, 2023 12:10
varsill
varsill approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥇

@bblaszkow06 bblaszkow06 merged commit ceee394 into master Jun 19, 2023
@bblaszkow06 bblaszkow06 deleted the fix_idr_in_two_slices_au_split branch June 19, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varsill varsill varsill approved these changes

@DominikWolek DominikWolek Awaiting requested review from DominikWolek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bblaszkow06 @varsill