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

Add timestamp generation for profiles baseline, constrained baseline #21

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

sgfn
Copy link
Member

@sgfn sgfn commented Feb 9, 2023

Timestamp generation for profiles which may contain B-frames not yet implemented.

@sgfn sgfn requested a review from varsill February 9, 2023 13:36
@sgfn sgfn self-assigned this Feb 9, 2023
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! I am really impressed by the quality of the code you have provided ;)
My only concern is about the prepare_actions_for_aus function - I think we are mixing up the terms "timestamp" and "ordering_number" (in case of both maybe_generate_timestamps and state.previous_timestamps). Timestamps might be linearly correlated with the ordering_numbers in case of the constant framerate, but we shouldn't use these terms interchangeably.
I would suggest the following changes:

  1. Add au_counter to the state, which would hold the number of access_units (frames) already processed
  2. Simplify prepare_actions_for_aus function - it could look like that:
   {actions, au_counter, profile} =
      Enum.reduce(aus, {[], state.au_counter, state.profile}, fn au, {actions_acc, cnt, profile} ->
        {sps_actions, profile} = maybe_parse_sps(au, state, profile)
        {pts, dts} = prepare_timestamp(buffer_pts, buffer_dts, state, profile, cnt)
        {actions_acc ++ sps_actions ++ [{:buffer, {:output, wrap_into_buffer(au, pts, dts)}}], cnt + 1,
         profile}
      end)

    state =
      maybe_update_state(buffer_pts, buffer_dts, au_count, profile, state)
    {actions, state}
  1. prepare_timestamp would hold the whole logic of creating/rewritting of the timestamps (perhaps it might be worth to provide three different implementations, based on the state.mode)

lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
test/integration/test_source.ex Outdated Show resolved Hide resolved
test/test_helper.exs Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
@sgfn
Copy link
Member Author

sgfn commented Feb 10, 2023

@varsill Thanks for the review and the kind words :)) hope to have addressed all of your concerns with these fixes

@sgfn sgfn requested a review from varsill February 10, 2023 13:14
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.

Yeah, that's what I have meant ;)
There are some NITs you can consider implementing and one issue concerning the functionality - I think with the parser working in :nalu_aligned mode the first frame would be send with pts: nil and dts: nil

lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
@sgfn
Copy link
Member Author

sgfn commented Feb 10, 2023

Good catch -- fixed. Thanks

@sgfn sgfn marked this pull request as ready for review February 10, 2023 14:33
@sgfn sgfn requested a review from varsill February 10, 2023 14:33
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! 🥇

@sgfn sgfn merged commit 4d27b9b into master Feb 13, 2023
@sgfn sgfn deleted the timestamp-generation branch February 13, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants