[MS-51] Use file seeking in `Membrane.WAV.Serializer` #10

balins · 2022-03-15T10:37:27Z

If Membrane.File.SeekEvent is available, Membrane.WAV.Serializer will emit event that updates invalid WAV header. Otherwise, Membrane.WAV.Serializer works like before.

bblaszkow06 · 2022-03-16T09:45:07Z

test/serializer/serializer_test.exs

@@ -101,5 +102,38 @@ defmodule Membrane.WAV.SerializerTest do
      assert_sink_buffer(pid, :sink, %Buffer{payload: ^payload})
      Pipeline.stop_and_terminate(pid, blocking?: true)
    end
+
+    @tag :tmp_dir
+    test "create valid file when `update_header?` = `true`", %{tmp_dir: tmp_dir} do


The markdown syntax doesn't make sense here as it will never go through any markdown processor:

Suggested change

test "create valid file when `update_header?` = `true`", %{tmp_dir: tmp_dir} do

test "create valid file when 'update_header?' is enabled", %{tmp_dir: tmp_dir} do

It was rather an emphasis for the reader that this is some `variable`. But surely can be removed if we don't follow such convention.

bblaszkow06 · 2022-03-16T09:51:15Z

README.md

+The Serializer creates header with invalid blocks which depend on the file size. You can bypass this 
+using `Membrane.File.Sink` to save the file or fixing the header afterwards with `Membrane.WAV.Postprocessing`
+if you need to use any other sink.


This sounds like the element is broken and you can somehow work around the issue. I'd rephrase it to something mentioning, that if the element after serializer doesn't support seek events, you may work this around by disabling option in the element and using Postprocessing

bblaszkow06 · 2022-03-16T09:54:16Z

lib/membrane_wav/serializer.ex

@@ -32,6 +36,18 @@ defmodule Membrane.WAV.Serializer do
                Used when converting demand from buffers into bytes.
                """,
                default: 2048
+              ],
+              update_header?: [


To better communicate that this is not something usually done, I'd rename it to disable_seeking or disable_header_update.

bblaszkow06 · 2022-03-16T09:57:35Z

lib/membrane_wav/serializer.ex


    {:ok, state}
  end

  @impl true
  def handle_caps(:input, caps, _context, state) do
    buffer = %Buffer{payload: create_header(caps)}
-    state = %{state | header_created: true}
-
+    state = %{state | header_length: byte_size(buffer.payload)}


This might be the place to subtract the 8 bytes not included in header length

lib/membrane_wav/serializer.ex

bblaszkow06 · 2022-03-16T10:19:13Z

lib/membrane_wav/serializer.ex

+        require Membrane.Logger
+
+        Membrane.Logger.warn("""
+        Couldn't update WAV header as `Membrane.File.SeekEvent` module is not available (reason: #{inspect(reason)}).
+        You can use `Membrane.File.Sink` in your pipeline to correctly save the file or fix it later using
+        `Membrane.WAV.Postprocessing`.
+        """)


This should not go just with a warning that can be missed. In particular, the file plugin is now :test only and will "disappear" if running in MIX_ENV=dev. This scenario (enabled update and no 'seek' event is available) should raise with an error suggesting to disable the option and informing about consequences (i.e. the file won't have a valid header).

bblaszkow06 · 2022-03-16T10:25:18Z

mix.exs

@@ -36,7 +36,7 @@ defmodule Membrane.WAV.Plugin.Mixfile do
      {:ex_doc, "~> 0.26", only: :dev, runtime: false},
      {:dialyxir, "~> 1.1", only: :dev, runtime: false},
      {:credo, "~> 1.6", only: :dev, runtime: false},
-      {:membrane_file_plugin, "~> 0.7.0", only: :test},
+      {:membrane_file_plugin, "~> 0.8.0", only: :test},


If it's not available in 'dev', you won't be able to test it from iex. We can solve it by making the plugin an optional: true dependency

bblaszkow06 · 2022-03-16T11:50:49Z

lib/membrane_wav/serializer.ex

+  defp maybe_update_header_actions(%{header_length: header_length, data_length: data_length}) do
+    case Code.ensure_compiled(Membrane.File.SeekEvent) do
+      {:module, seek_event} ->
+        # subtracting 8 bytes as `file_length` field doesn't include "RIFF" header and the field itself
+        file_length = header_length + data_length - 8
+
+        [
+          event: {:output, struct!(seek_event, position: @file_length_offset)},
+          buffer: {:output, %Buffer{payload: <<file_length::32-little>>}},
+          event: {:output, struct!(seek_event, position: @data_length_offset)},
+          buffer: {:output, %Buffer{payload: <<data_length::32-little>>}}
+        ]
+
+      {:error, reason} ->
+        require Membrane.Logger
+
+        Membrane.Logger.warn("""
+        Couldn't update WAV header as `Membrane.File.SeekEvent` module is not available (reason: #{inspect(reason)}).
+        You can use `Membrane.File.Sink` in your pipeline to correctly save the file or fix it later using
+        `Membrane.WAV.Postprocessing`.
+        """)
+
+        []
+    end
+  end


This is invoked from an app, so ensure_loaded? should be enough. Also, if the check was moved outside, you could use the proper struct creation syntax and have compile-time check for the keys:

Suggested change

defp maybe_update_header_actions(%{header_length: header_length, data_length: data_length}) do

case Code.ensure_compiled(Membrane.File.SeekEvent) do

{:module, seek_event} ->

# subtracting 8 bytes as `file_length` field doesn't include "RIFF" header and the field itself

file_length = header_length + data_length - 8

[

event: {:output, struct!(seek_event, position: @file_length_offset)},

buffer: {:output, %Buffer{payload: <<file_length::32-little>>}},

event: {:output, struct!(seek_event, position: @data_length_offset)},

buffer: {:output, %Buffer{payload: <<data_length::32-little>>}}

]

{:error, reason} ->

require Membrane.Logger

Membrane.Logger.warn("""

Couldn't update WAV header as `Membrane.File.SeekEvent` module is not available (reason: #{inspect(reason)}).

You can use `Membrane.File.Sink` in your pipeline to correctly save the file or fix it later using

`Membrane.WAV.Postprocessing`.

""")

[]

end

end

if Code.ensure_loaded?(Membrane.File.SeekEvent) do

defp maybe_update_header_actions(%{

update_header?: true,

header_length: header_length,

data_length: data_length

}) do

# subtracting 8 bytes as `file_length` field doesn't include "RIFF" header and the field itself

file_length = header_length + data_length - 8

[

event: {:output, %Membrane.File.SeekEvent{position: @file_length_offset}},

buffer: {:output, %Buffer{payload: <<file_length::32-little>>}},

event: {:output, %Membrane.File.SeekEvent{position: @data_length_offset}},

buffer: {:output, %Buffer{payload: <<data_length::32-little>>}}

]

end

else

defp maybe_update_header_actions(%{update_header?: true}) do

# Suppress warnings

_offset = @file_length_offset

_offset = @data_length_offset

raise "Unable to update WAV file header using seek events. [...]"

end

end

This can be verified by replacing position with positions (assuming file plugin is still a test-only dependency) will result in compilation error for test env and clean compilation on dev

Surely that's way better!

Co-authored-by: Bartosz Błaszków <bbartosz06@gmail.com>

bblaszkow06

test/serializer/serializer_test.exs

balins added 5 commits March 14, 2022 17:09

add SerializerBin

7dfad2e

add SerializerBinTest

c9f33b6

update readme

b91b198

move seeking logic directly into WAV.Serializer

878a576

use handle_process_list/4 in Serializer

b7733c7

balins added the enhancement New feature or request label Mar 15, 2022

balins requested a review from bblaszkow06 March 15, 2022 10:37

balins self-assigned this Mar 15, 2022

bblaszkow06 requested changes Mar 16, 2022

View reviewed changes

balins and others added 2 commits March 17, 2022 08:39

Apply suggestions from code review

afde518

Co-authored-by: Bartosz Błaszków <bbartosz06@gmail.com>

adjust to CR suggestions

67802e6

balins requested a review from bblaszkow06 March 17, 2022 09:31

remove unnecessary match on disable_seeking

52a0659

bblaszkow06 approved these changes Mar 17, 2022

View reviewed changes

test/serializer/serializer_test.exs Show resolved Hide resolved

balins merged commit 7dde7cd into master Mar 18, 2022

balins deleted the MS-51-wav-plugin-use-seeking-in-file-sink branch March 18, 2022 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MS-51] Use file seeking in `Membrane.WAV.Serializer` #10

[MS-51] Use file seeking in `Membrane.WAV.Serializer` #10

balins commented Mar 15, 2022

bblaszkow06 Mar 16, 2022

balins Mar 17, 2022

bblaszkow06 Mar 16, 2022

bblaszkow06 Mar 16, 2022

bblaszkow06 Mar 16, 2022

bblaszkow06 Mar 16, 2022

bblaszkow06 Mar 16, 2022

bblaszkow06 Mar 16, 2022

balins Mar 17, 2022

bblaszkow06 left a comment

	test "create valid file when `update_header?` = `true`", %{tmp_dir: tmp_dir} do
	test "create valid file when 'update_header?' is enabled", %{tmp_dir: tmp_dir} do

[MS-51] Use file seeking in Membrane.WAV.Serializer #10

[MS-51] Use file seeking in Membrane.WAV.Serializer #10

Conversation

balins commented Mar 15, 2022

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

bblaszkow06 left a comment

Choose a reason for hiding this comment

[MS-51] Use file seeking in `Membrane.WAV.Serializer` #10

[MS-51] Use file seeking in `Membrane.WAV.Serializer` #10