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

Timestamps for B frames #38

Merged
merged 14 commits into from
Aug 16, 2023
Merged

Timestamps for B frames #38

merged 14 commits into from
Aug 16, 2023

Conversation

mat-hek
Copy link
Member

@mat-hek mat-hek commented Aug 4, 2023

TODO:

  • calculate timestamps for B frames
  • refactor prepare_actions_for_aus
  • determine if we need the max_frame_reorder option
  • refactor timestamp rewriting
  • hide timestamp generation under additional flag
  • add async: true to tests

@mat-hek mat-hek requested a review from varsill as a code owner August 4, 2023 15:36
@mat-hek mat-hek changed the title Timestamps b frames Timestamps for B frames Aug 4, 2023
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser/au_splitter.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser/au_splitter.ex Outdated Show resolved Hide resolved
lib/membrane_h264_plugin/parser/au_timestamp_generator.ex Outdated Show resolved Hide resolved
@@ -319,40 +327,62 @@ defmodule Membrane.H264.Parser do
output_alignment: state.output_alignment
)

{[stream_format: {:output, fmt}], fmt.profile}
max_frame_reorder = if fmt.profile in [:baseline, :constrained_baseline], do: 0, else: 15
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, that can cause some problems when the profile is changed during the stream run

test/integration/timestamp_generation_test.exs Outdated Show resolved Hide resolved
test/integration/timestamp_generation_test.exs Outdated Show resolved Hide resolved
@mat-hek mat-hek force-pushed the timestamps-b-frames branch 2 times, most recently from 3cf513c to 58e881b Compare August 8, 2023 14:15
@mat-hek mat-hek requested a review from varsill August 9, 2023 08:21
description: """
Generates timestamps based on given `framerate`.

This option works only when `Membrane.RemoteStream` format arrives on
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add some warning when other format arrives and generate_best_effort_timestamps: true is set, that the option has no effect

Copy link
Member Author

@mat-hek mat-hek Aug 10, 2023

Choose a reason for hiding this comment

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

We could, but then we would warn if somebody had the same parser config for different inputs

lib/membrane_h264_plugin/parser.ex Show resolved Hide resolved
lib/membrane_h264_plugin/parser.ex Outdated Show resolved Hide resolved
@mat-hek mat-hek requested a review from varsill August 11, 2023 10:18
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.

Just one small thing to consider ;)

%{au_counter: au_counter, key_frame_au_idx: key_frame_au_idx} = state
first_vcl_nalu = Enum.find(au, &NALuTypes.is_vcl_nalu_type(&1.type))
{poc, state} = calculate_poc(first_vcl_nalu, state)
key_frame_au_idx = if poc == 0, do: au_counter, else: key_frame_au_idx
pts = div((key_frame_au_idx + poc) * seconds * Membrane.Time.second(), frames)
max_frame_reorder = if Map.get(config, :add_dts_offset, true), do: 15, else: 0
Copy link
Contributor

@varsill varsill Aug 16, 2023

Choose a reason for hiding this comment

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

let's parametrize that magic number 15. I would also consider "parsing" that add_dts_offset option in handle_init (by which I mean that we can set its default value there)

@mat-hek mat-hek force-pushed the timestamps-b-frames branch 2 times, most recently from 7370eec to a91eaf1 Compare August 16, 2023 13:10
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 🥇

@mat-hek mat-hek merged commit e6567b6 into master Aug 16, 2023
3 checks passed
@mat-hek mat-hek deleted the timestamps-b-frames branch August 16, 2023 13:38
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

2 participants