Bug fix: Complete frames as soon as it is possible

It should not be necessary to wait for the subsequent sync word to
determine that the current frame is complete - this can be assured
simply back checking the expected frame size against the actual bytes.

This ensures that completed frames are made available immediately, and
avoids dropping a single frame from every binary stream that is
processed.

lib/mpeg_audio_frame_parser.ex

@@ -36,9 +36,7 @@ defmodule MPEGAudioFrameParser do
 
   ## Example
 
-  Using a faked 128kbps 44.1k stereo MP3 frame. Note at least two headers must
+  Using a faked 128kbps 44.1k stereo MP3 frame:
-  be processed to return a single MP3 frame - otherwise frame completeness
-  couldn't be assured:
 
       iex> packet = <<0b11111111111_11_01_0_1001_00_0_0_00_00_0_0_00::size(32), 1::size(3304)>>
       ...> {:ok, _pid} = MPEGAudioFrameParser.start_link()
@@ -74,9 +72,7 @@ defmodule MPEGAudioFrameParser do
 
   ## Example
 
-  Using a faked 128kbps 44.1k stereo MP3 frame. Note at least two headers must
+  Using a faked 128kbps 44.1k stereo MP3 frame:
-  be processed to return a single MP3 frame - otherwise frame completeness
-  couldn't be assured:
 
       iex> packet = <<0b11111111111_11_01_0_1001_00_0_0_00_00_0_0_00::size(32), 1::size(3304)>>
       ...> {:ok, _pid} = MPEGAudioFrameParser.start_link()

lib/mpeg_audio_frame_parser/impl.ex

@@ -28,6 +28,12 @@ defmodule MPEGAudioFrameParser.Impl do
 
   # Private Functions
 
+  # Synced, and the current frame is complete:
+  defp process_bytes(%{current_frame: %Frame{complete: true}} = state, packet) do
+    frames = [state.current_frame | state.frames]
+    process_bytes(%{state | current_frame: nil, frames: frames}, packet)
+  end
+
   # No data left, or not enough to be able to validate next frame. Return:
   defp process_bytes(state, packet)
   when bit_size(packet) < 32
@@ -62,13 +68,6 @@ defmodule MPEGAudioFrameParser.Impl do
     process_bytes(%{state | current_frame: nil}, rest)
   end
 
-  # Synced, and the current frame is complete:
-  defp process_bytes(%{current_frame: %Frame{complete: true}} = state, packet) do
-    frames = [state.current_frame | state.frames]
-    new_state = %{state | current_frame: nil, frames: frames}
-    process_bytes(new_state, packet)
-  end
-
   # Synced, current frame not complete and we have bytes available. Add bytes to frame:
   defp process_bytes(%{current_frame: %Frame{complete: false}} = state, packet) do
     {:ok, frame, rest} = Frame.add_bytes(state.current_frame, packet)

test/integration_test.exs

@@ -3,27 +3,27 @@ defmodule MPEGAudioFrameParserIntegrationTest do
 
   test "128kbps 44100hz MP3" do
     MPEGAudioFrameParser.start_link()
-    assert count_frames("test/fixtures/test_128_44100.mp3") == 253
+    assert count_frames("test/fixtures/test_128_44100.mp3") == 254
   end
 
   test "64kbps 12000hz MP3" do
     MPEGAudioFrameParser.start_link()
-    assert count_frames("test/fixtures/test_64_12000.mp3") == 139
+    assert count_frames("test/fixtures/test_64_12000.mp3") == 140
   end
 
   test "160kbps 24000hz MP3" do
     MPEGAudioFrameParser.start_link()
-    assert count_frames("test/fixtures/test_160_24000.mp3") == 276
+    assert count_frames("test/fixtures/test_160_24000.mp3") == 277
   end
 
   test "128kbps 44100hz MP3 with CRC protection" do
     MPEGAudioFrameParser.start_link()
-    assert count_frames("test/fixtures/test_128_44100_crc_protection.mp3") == 253
+    assert count_frames("test/fixtures/test_128_44100_crc_protection.mp3") == 254
   end
 
   test "64kbps 22050hz MP2" do
     MPEGAudioFrameParser.start_link()
-    assert count_frames("test/fixtures/test_64_22050.mp2") == 125
+    assert count_frames("test/fixtures/test_64_22050.mp2") == 126
   end
 
   defp count_frames(path) do

test/mpeg_audio_frame_parser/impl_test.exs

@@ -13,12 +13,9 @@ defmodule MPEGAudioFrameParser.ImplTest do
 
     test "handles a single frame at the start of a packet" do
       {:ok, state} = init()
-
       {:ok, state} = add_packet(state, @frame1)
 
-      assert state.current_frame.data == @frame1
+      assert %{current_frame: nil, frames: [%{data: @frame1}]} = state
-      assert state.current_frame.complete
-      assert state.frames == []
     end
 
     test "handles a single frame in the middle of a packet" do
@@ -27,9 +24,7 @@ defmodule MPEGAudioFrameParser.ImplTest do
       packet = <<0, 1, 2, 3, @frame1::binary>>
       {:ok, state} = add_packet(state, packet)
 
-      assert state.current_frame.data == @frame1
+      assert %{current_frame: nil, frames: [%{data: @frame1}]} = state
-      assert state.current_frame.complete
-      assert state.frames == []
     end
 
     test "ignores a packet that includes no valid frames at all" do
@@ -37,8 +32,7 @@ defmodule MPEGAudioFrameParser.ImplTest do
 
       {:ok, state} = add_packet(state, <<1::size(10240)>>)
 
-      assert state.frames == []
+      assert %{current_frame: nil, frames: []} = state
-      assert state.current_frame == nil
     end
 
     test "handles two frames in consecutive packets" do
@@ -47,9 +41,7 @@ defmodule MPEGAudioFrameParser.ImplTest do
       {:ok, state} = add_packet(state, @frame1)
       {:ok, state} = add_packet(state, @frame3)
 
-      assert length(state.frames) == 1
+      assert %{current_frame: nil, frames: [%{data: @frame3}, %{data: @frame1}]} = state
-      assert List.first(state.frames).data == @frame1
-      assert state.current_frame.data == @frame3
     end
 
     test "handles a frame split unevenly across consecutive packets" do
@@ -57,13 +49,9 @@ defmodule MPEGAudioFrameParser.ImplTest do
 
       part1 = :binary.part(@frame1, {0, 256})
       part2 = :binary.part(@frame1, {byte_size(@frame1), -(byte_size(@frame1) - 256)})
-      part3 = :binary.part(@frame3, {0, 256})
 
-      packet = <<8, 0, 1, 0, 0, 0, 7, 90, 93, part1::binary>>
+      {:ok, state} = add_packet(state, <<0, 1, 2, 3, part1::binary>>)
-      {:ok, state} = add_packet(state, packet)
+      {:ok, state} = add_packet(state, part2)
-
-      packet = <<part2::binary, part3::binary>>
-      {:ok, state} = add_packet(state, packet)
 
       assert length(state.frames) == 1
       assert List.first(state.frames).data == @frame1
@@ -71,23 +59,21 @@ defmodule MPEGAudioFrameParser.ImplTest do
 
     test "handles three frames in a single packet" do
       {:ok, state} = init()
-
       {:ok, state} = add_packet(state, <<@frame1::binary, @frame1::binary, @frame1::binary>>)
 
-      assert length(state.frames) == 2
+      assert length(state.frames) == 3
-      assert Enum.map(state.frames, & &1.data) == [@frame1, @frame1]
+      assert Enum.map(state.frames, & &1.data) == [@frame1, @frame1, @frame1]
     end
 
     test "handles three frames in consecutive packets" do
       {:ok, state} = init()
-
       {:ok, state} = add_packet(state, @frame3)
       {:ok, state} = add_packet(state, @frame3)
       {:ok, state} = add_packet(state, @frame3)
 
-      assert length(state.frames) == 2
+      assert length(state.frames) == 3
-      assert Enum.map(state.frames, & &1.data) == [@frame3, @frame3]
+      assert Enum.map(state.frames, & &1.data) == [@frame3, @frame3, @frame3]
-      assert state.current_frame.data == @frame3
+      assert is_nil(state.current_frame)
     end
   end
 
@@ -104,8 +90,6 @@ defmodule MPEGAudioFrameParser.ImplTest do
       {:ok, state} = init()
 
       {:ok, state} = add_packet(state, @frame1)
-      {:ok, state} = add_packet(state, @frame1)
-
       {:ok, frame, state} = pop_frame(state)
 
       assert frame.data == @frame1
@@ -117,7 +101,6 @@ defmodule MPEGAudioFrameParser.ImplTest do
 
       {:ok, state} = add_packet(state, @frame1)
       {:ok, state} = add_packet(state, @frame2)
-      {:ok, state} = add_packet(state, @frame1)
 
       {:ok, frame, state} = pop_frame(state)
       assert frame.data == @frame1