Repository navigation
AV1 muxer - #125
AV1 muxer#125
Conversation
There was a problem hiding this comment.
Please refactor as we spoke on DC https://discord.com/channels/464786597288738816/1446470754995671092/1473283552262230213
Also, make it depend on https://github.com/membraneframework/membrane_av1_format
8ce056a to
e02a7c3
Compare
Noarkhh
left a comment
There was a problem hiding this comment.
Hi, thank you for the contribution! Sorry this took so long, but I was during the development of membrane_av1_plugin and wanted to finish working on the encoder and decoder there, so that the API is somewhat stable.
During said development there were changes to Membrane.AV1 format and I added suggestions conforming to the new version.
Also please add tests in test/membrane_mp4/demuxer/isom/integration_test.exs.
| profile: format.profile || 0, | ||
| level_idx: level_string_to_idx(format.level) || 8, | ||
| tier: format.tier || 0, |
There was a problem hiding this comment.
In the last release of membrane_av1_format, these three fields are now all atoms. They can be converted to their respective seq_ values with utility functions from Membrane.AV1 module:
profile_to_seq_profile/1, level_to_seq_level_idx/1 and tier_to_seq_tier/1
| # Convert AV1 level string to level index | ||
| defp level_string_to_idx(nil), do: nil | ||
| defp level_string_to_idx("2.0"), do: 0 | ||
| defp level_string_to_idx("2.1"), do: 1 | ||
| defp level_string_to_idx("3.0"), do: 4 | ||
| defp level_string_to_idx("3.1"), do: 5 | ||
| defp level_string_to_idx("4.0"), do: 8 | ||
| defp level_string_to_idx("4.1"), do: 9 | ||
| defp level_string_to_idx("5.0"), do: 12 | ||
| defp level_string_to_idx("5.1"), do: 13 | ||
| defp level_string_to_idx("5.2"), do: 14 | ||
| defp level_string_to_idx("5.3"), do: 15 | ||
| defp level_string_to_idx("6.0"), do: 16 | ||
| defp level_string_to_idx("6.1"), do: 17 | ||
| defp level_string_to_idx("6.2"), do: 18 | ||
| defp level_string_to_idx("6.3"), do: 19 | ||
| defp level_string_to_idx(_), do: nil | ||
|
|
There was a problem hiding this comment.
These will no longer be necessary
| }, | ||
| %Membrane.Opus{self_delimiting?: false} | ||
| %Membrane.Opus{self_delimiting?: false}, | ||
| %Membrane.AV1{} |
There was a problem hiding this comment.
| %Membrane.AV1{} | |
| %Membrane.AV1{alignment: :tu} |
| | {:av01, | ||
| %{profile: non_neg_integer(), level: String.t() | nil, tier: non_neg_integer()}} |
There was a problem hiding this comment.
The convention is to use raw values from the format, so I propose to do it for level as well
| | {:av01, | |
| %{profile: non_neg_integer(), level: String.t() | nil, tier: non_neg_integer()}} | |
| | {:av01, | |
| %{profile: non_neg_integer(), level: non_neg_integer(), tier: non_neg_integer()}} |
There was a problem hiding this comment.
This also needs to be added to Membrane.CMAF.Track.t() in membrane_cmaf_format package, if you could open a PR there that would be great
| profile: format.profile || 0, | ||
| level: format.level, | ||
| tier: format.tier || 0 |
There was a problem hiding this comment.
Same as before, use utility functions from Membrane.AV1
|
|
||
| # partial segments for the following 8 seconds without a keyframe | ||
| for _iteration <- 1..16 do | ||
| for _ <- 1..16 do |
There was a problem hiding this comment.
It was like this previously because we have credo warn when a variable is named _
| for _ <- 1..16 do | |
| for _iteration <- 1..16 do |
| {:membrane_h264_format, "~> 0.6.1"}, | ||
| {:membrane_h265_format, "~> 0.2.0"}, | ||
| {:membrane_opus_format, "~> 0.3.0"}, | ||
| {:membrane_av1_format, "~> 0.1.0"}, |
There was a problem hiding this comment.
| {:membrane_av1_format, "~> 0.1.0"}, | |
| {:membrane_av1_format, "~> 0.3.0"}, |
73b2af4 to
9a96a0d
Compare
Noarkhh
left a comment
There was a problem hiding this comment.
looks good overall, but please address these naming suggestions and add some muxer tests
| alias Membrane.MP4.MovieBox.SampleTableBox | ||
| alias Membrane.MP4.Track.SampleTable | ||
|
|
||
| @temporal_delimiter <<0x12, 0x00>> |
There was a problem hiding this comment.
| @temporal_delimiter <<0x12, 0x00>> | |
| @av1_temporal_delimiter_obu <<0x12, 0x00>> |
| defp strip_temporal_delimiter(<<0x12, 0x00, rest::binary>>), do: strip_temporal_delimiter(rest) | ||
| defp strip_temporal_delimiter(payload), do: payload |
There was a problem hiding this comment.
| defp strip_temporal_delimiter(<<0x12, 0x00, rest::binary>>), do: strip_temporal_delimiter(rest) | |
| defp strip_temporal_delimiter(payload), do: payload | |
| defp strip_av1_temporal_delimiter_obu(<<0x12, 0x00, rest::binary>>), do: strip_temporal_delimiter(rest) | |
| defp strip_av1_temporal_delimiter_obu(payload), do: payload |
No description provided.