Forward timestamps #57

bartkrak · 2024-03-12T01:16:23Z

partially solves: membraneframework/membrane_core#735

spscream · 2024-03-12T13:15:09Z

I have crash with the following pipeline opus_decode -> convert 48khz to 32khz -> g7221 encoder -> rtp, this merge fixes it. Thanks

FelonEkonom

Not sure, if passing pts from input to output is a good approach here, I recommend to ask somebody more familiar with this plugin, if it doesn't require more sophisticated logic (e.g. passing on output pts of the first buffer, which payload was part of state.queue)

FelonEkonom · 2024-03-12T13:17:22Z

lib/membrane_ffmpeg_swresample_plugin/converter.ex

+      |> Map.merge(%{
+        current_pts: pts
+      })


Suggested change

|> Map.merge(%{

current_pts: pts

})

|> Map.put(:current_pts, pts)

@bartkrak actually, the right approach would be to add this field in handle_init and do %{state | current_pts: pts}. Adding fields to state in various callbacks makes it hard to follow.

mat-hek

As we spoke, we must take into account the fact that the resampler may queue the stream

mat-hek · 2024-03-25T16:12:19Z

lib/membrane_ffmpeg_swresample_plugin/converter.ex

+    output_frame_size = RawAudio.frame_size(state.output_stream_format)
+    ratio = output_frame_size / input_frame_size
+
+    expected_output_frames_count = byte_size(payload) / input_frame_size * ratio


I think we should round to an integer here

mat-hek · 2024-03-25T16:34:13Z

lib/membrane_ffmpeg_swresample_plugin/converter.ex

+          Map.update!(state, :pts_queue, fn _pts_queue ->
+            [{out_pts, expected_frames - converted_frames_count}] ++ rest
+          end)


this can be %{state | pts_queue: ...

mat-hek · 2024-03-25T16:34:28Z

lib/membrane_ffmpeg_swresample_plugin/converter.ex

+    cond do
+      converted_frames_count < expected_frames ->
+        state =
+          Map.update!(state, :pts_queue, fn _pts_queue ->
+            [{out_pts, expected_frames - converted_frames_count}] ++ rest
+          end)
+
+        {state, out_pts}
+
+      converted_frames_count >= expected_frames ->
+        {%{state | pts_queue: rest}, out_pts}
+    end


this seems like an if

mat-hek · 2024-03-25T16:35:30Z

test/membrane_ffmpeg_swresample_plugin/converter_test.exs

@@ -157,7 +157,14 @@ defmodule Membrane.FFmpeg.SWResample.ConverterTest do

      assert {[], new_state} = @module.handle_buffer(:input, buffer, nil, state)

-      assert new_state == %{state | queue: payload}
+      # this is necesary to ignore state.pts_queue in the assertion


you can set it to nil on both sides then

mat-hek · 2024-03-25T16:36:04Z

test/membrane_ffmpeg_swresample_plugin/converter_test.exs

@@ -178,5 +186,21 @@ defmodule Membrane.FFmpeg.SWResample.ConverterTest do
      assert actions == [buffer: {:output, %Membrane.Buffer{payload: result}}]
      assert new_state == %{state | queue: <<0::2*8>>}
    end
+
+    # test "timestamps forward test", %{state: initial_state} do


mat-hek · 2024-03-25T16:36:16Z

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

+
+  test "pts forward test" do
+    input_stream_format = %RawAudio{sample_format: :s16le, sample_rate: 16_000, channels: 2}
+    # input_stream_format = %RawAudio{sample_format: :u8, sample_rate: 8_000, channels: 1}


more leftovers

mat-hek · 2024-03-26T09:01:16Z

lib/membrane_ffmpeg_swresample_plugin/converter.ex

@@ -128,7 +128,7 @@ defmodule Membrane.FFmpeg.SWResample.Converter do
    output_frame_size = RawAudio.frame_size(state.output_stream_format)
    ratio = output_frame_size / input_frame_size

-    expected_output_frames_count = byte_size(payload) / input_frame_size * ratio
+    expected_output_frames_count = (byte_size(payload) / input_frame_size * ratio) |> trunc()


I think it's necessary to use round. Otherwise we'd expect 2137 frames if the float result is 2137.99

mat-hek · 2024-03-26T09:02:45Z

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

+    output_stream_format = %RawAudio{sample_format: :s32le, sample_rate: 32_000, channels: 2}
+
+    # 32 frames * 2048 bytes
+    fixture_path = Path.expand(Path.join(__DIR__, "/../fixtures/input_s16le_stereo_16khz.raw"))


the run path in tests is always the project root, so it's safe to do

Suggested change

fixture_path = Path.expand(Path.join(__DIR__, "/../fixtures/input_s16le_stereo_16khz.raw"))

fixture_path = "test/fixtures/input_s16le_stereo_16khz.raw"

mat-hek · 2024-03-26T09:06:29Z

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

+
+    Enum.each(0..30, fn index ->
+      assert_sink_buffer(pipeline, :sink, %Membrane.Buffer{pts: out_pts})
+      assert out_pts == index * 31_250_000


let's make this magic number a variable

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

mat-hek · 2024-03-26T09:12:22Z

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

+  @spec split_binary(binary(), list(binary())) :: list(binary())
+  def split_binary(binary, acc \\ [])
+
+  def split_binary(<<binary::binary-size(2048), rest::binary>>, acc) do


Would it be hard to have the chunk length differ from buffer to buffer? I guess we could have better coverage that way

but if it's too much work let's leave it as is

mat-hek · 2024-03-29T12:46:08Z

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

+    |> Enum.map(fn {payload, index} ->
+      %Membrane.Buffer{
+        payload: payload,
+        pts: index * 31_250_000


actually pts_multiplayer can be a module attribute and you can use it here as well

mat-hek · 2024-03-29T12:46:39Z

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

+      assert out_pts == index * pts_multiplier
+    end)
+
+    assert_sink_buffer(pipeline, :sink, %Membrane.Buffer{pts: 937_500_000})


Suggested change

assert_sink_buffer(pipeline, :sink, %Membrane.Buffer{pts: 937_500_000})

assert_sink_buffer(pipeline, :sink, %Membrane.Buffer{pts: 30 * pts_multiplier)

using multiplication inside a match throws compile error

FelonEkonom · 2024-04-02T15:37:03Z

test/membrane_ffmpeg_swresample_plugin/pts_forward_test.exs

+    assert_start_of_stream(pipeline, :sink)
+    assert_sink_buffer(pipeline, :sink, _buffer)


The second assertion makes the first one redundant

FelonEkonom · 2024-04-02T15:47:26Z

lib/membrane_ffmpeg_swresample_plugin/converter.ex

+    input_frame_size = RawAudio.frame_size(state.input_stream_format)
+    output_frame_size = RawAudio.frame_size(state.output_stream_format)
+    ratio = output_frame_size / input_frame_size
+
+    expected_output_frames_count = (byte_size(payload) / input_frame_size * ratio) |> round()


It is more precise to write (byte_size(payload) * output_frame_size / (input_frame_size * input_frame_size)) because of limitted accuracy of floats

FelonEkonom · 2024-04-02T15:54:12Z

lib/membrane_ffmpeg_swresample_plugin/converter.ex

@@ -212,4 +233,15 @@ defmodule Membrane.FFmpeg.SWResample.Converter do
      {:error, reason} -> raise "Error while flushing converter: #{inspect(reason)}"
    end
  end
+
+  defp update_pts_queue(state, converted_frames_count) do
+    {[{out_pts, expected_frames}], rest} = Enum.split(state.pts_queue, 1)


You can just match

Suggested change

{[{out_pts, expected_frames}], rest} = Enum.split(state.pts_queue, 1)

[{out_pts, expected_frames} | rest] = state.pts_queue

bartkrak added 3 commits March 12, 2024 01:04

pts forward, test

81933ba

end of stream pts

0ac7da5

format

58039a7

bartkrak requested review from FelonEkonom and mat-hek March 12, 2024 01:17

bartkrak mentioned this pull request Mar 12, 2024

Find elements that ignore timestamps while they shouldn't membraneframework/membrane_core#735

Closed

9 tasks

FelonEkonom requested changes Mar 12, 2024

View reviewed changes

refactor

c39aa63

mat-hek requested changes Mar 15, 2024

View reviewed changes

bartkrak added 4 commits March 19, 2024 23:19

almost there

bffb6fb

one test fixed

e0285c7

might be good

3ccd0bf

cleanup, tests adjusted

f3de578

bartkrak requested a review from mat-hek March 21, 2024 18:06

mat-hek requested changes Mar 25, 2024

View reviewed changes

bartkrak added 2 commits March 25, 2024 19:35

pts test update

cd2486d

little refactor

7e7c7bb

bartkrak requested a review from mat-hek March 25, 2024 18:45

mat-hek requested changes Mar 26, 2024

View reviewed changes

refactor

d78762e

bartkrak requested a review from mat-hek March 26, 2024 16:56

mat-hek approved these changes Mar 29, 2024

View reviewed changes

bartkrak added 2 commits April 2, 2024 14:29

cosmetic refactor

1473e39

version bump

9ebe020

bartkrak requested a review from FelonEkonom April 2, 2024 12:39

FelonEkonom requested changes Apr 2, 2024

View reviewed changes

refactor

1751941

bartkrak requested a review from FelonEkonom April 2, 2024 18:46

FelonEkonom approved these changes Apr 3, 2024

View reviewed changes

bartkrak merged commit 47f76bd into master Apr 3, 2024
3 checks passed

bartkrak deleted the forward_timestamps branch April 3, 2024 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward timestamps #57

Forward timestamps #57

bartkrak commented Mar 12, 2024 •

edited

Loading

spscream commented Mar 12, 2024

FelonEkonom left a comment

FelonEkonom Mar 12, 2024

mat-hek Mar 15, 2024

mat-hek left a comment

mat-hek Mar 25, 2024

mat-hek Mar 25, 2024

mat-hek Mar 25, 2024

mat-hek Mar 25, 2024

mat-hek Mar 25, 2024

mat-hek Mar 25, 2024

mat-hek Mar 26, 2024

mat-hek Mar 26, 2024

mat-hek Mar 26, 2024

mat-hek Mar 26, 2024

mat-hek Mar 26, 2024

mat-hek Mar 29, 2024

mat-hek Mar 29, 2024

bartkrak Apr 2, 2024

FelonEkonom Apr 2, 2024

FelonEkonom Apr 2, 2024

FelonEkonom Apr 2, 2024

	fixture_path = Path.expand(Path.join(__DIR__, "/../fixtures/input_s16le_stereo_16khz.raw"))
	fixture_path = "test/fixtures/input_s16le_stereo_16khz.raw"

	assert_sink_buffer(pipeline, :sink, %Membrane.Buffer{pts: 937_500_000})
	assert_sink_buffer(pipeline, :sink, %Membrane.Buffer{pts: 30 * pts_multiplier)

		assert_start_of_stream(pipeline, :sink)
		assert_sink_buffer(pipeline, :sink, _buffer)

	{[{out_pts, expected_frames}], rest} = Enum.split(state.pts_queue, 1)
	[{out_pts, expected_frames} \| rest] = state.pts_queue

Forward timestamps #57

Forward timestamps #57

Conversation

bartkrak commented Mar 12, 2024 • edited Loading

spscream commented Mar 12, 2024

FelonEkonom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-hek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartkrak commented Mar 12, 2024 •

edited

Loading