Fixing caps being sent with each buffer and addition of credo (#28)

* Fixing too many caps and adding credo

* Making credo happy

* Update credo

* Add comment explaining disabling the credo for `Membrane.Opus.Util.parse_configuration/1`

* Fix documentation

* Don't calculate length twice

.circleci/config.yml

@@ -1,8 +1,8 @@
 version: 2.0
 jobs:
-  build:
+  test:
     docker:
-      - image: membrane/membrane:latest
+      - image: membraneframeworklabs/docker_membrane:latest
         environment:
           MIX_ENV: test
 
@@ -11,5 +11,28 @@ jobs:
     steps:
       - checkout
       - run: mix deps.get
-      - run: mix format --check-formatted
+      - run: mix compile --force --warnings-as-errors
       - run: mix test
+
+  lint:
+    docker:
+      - image: membraneframeworklabs/docker_membrane:latest
+        environment:
+          MIX_ENV: dev
+
+    working_directory: ~/app
+
+    steps:
+      - checkout
+      - run: mix deps.get
+      - run: mix format --check-formatted
+      - run: mix compile
+      - run: mix credo
+      - run: mix docs && mix docs 2>&1 | (! grep -q "warning:")
+
+workflows:
+  version: 2
+  build:
+    jobs:
+      - test
+      - lint

.credo.exs

@@ -0,0 +1,187 @@
+# This file contains the configuration for Credo and you are probably reading
+# this after creating it with `mix credo.gen.config`.
+#
+# If you find anything wrong or unclear in this file, please report an
+# issue on GitHub: https://github.com/rrrene/credo/issues
+#
+%{
+  #
+  # You can have as many configs as you like in the `configs:` field.
+  configs: [
+    %{
+      #
+      # Run any config using `mix credo -C <name>`. If no config name is given
+      # "default" is used.
+      #
+      name: "default",
+      #
+      # These are the files included in the analysis:
+      files: %{
+        #
+        # You can give explicit globs or simply directories.
+        # In the latter case `**/*.{ex,exs}` will be used.
+        #
+        included: [
+          "lib/",
+          "src/",
+          "test/",
+          "web/",
+          "apps/*/lib/",
+          "apps/*/src/",
+          "apps/*/test/",
+          "apps/*/web/"
+        ],
+        excluded: [~r"/_build/", ~r"/deps/", ~r"/node_modules/"]
+      },
+      #
+      # Load and configure plugins here:
+      #
+      plugins: [],
+      #
+      # If you create your own checks, you must specify the source files for
+      # them here, so they can be loaded by Credo before running the analysis.
+      #
+      requires: [],
+      #
+      # If you want to enforce a style guide and need a more traditional linting
+      # experience, you can change `strict` to `true` below:
+      #
+      strict: false,
+      #
+      # To modify the timeout for parsing files, change this value:
+      #
+      parse_timeout: 5000,
+      #
+      # If you want to use uncolored output by default, you can change `color`
+      # to `false` below:
+      #
+      color: true,
+      #
+      # You can customize the parameters of any check by adding a second element
+      # to the tuple.
+      #
+      # To disable a check put `false` as second element:
+      #
+      #     {Credo.Check.Design.DuplicatedCode, false}
+      #
+      checks: [
+        #
+        ## Consistency Checks
+        #
+        {Credo.Check.Consistency.ExceptionNames, []},
+        {Credo.Check.Consistency.LineEndings, []},
+        {Credo.Check.Consistency.ParameterPatternMatching, []},
+        {Credo.Check.Consistency.SpaceAroundOperators, []},
+        {Credo.Check.Consistency.SpaceInParentheses, []},
+        {Credo.Check.Consistency.TabsOrSpaces, []},
+
+        #
+        ## Design Checks
+        #
+        # You can customize the priority of any check
+        # Priority values are: `low, normal, high, higher`
+        #
+        {Credo.Check.Design.AliasUsage,
+         [priority: :low, if_nested_deeper_than: 2, if_called_more_often_than: 0]},
+        # You can also customize the exit_status of each check.
+        # If you don't want TODO comments to cause `mix credo` to fail, just
+        # set this value to 0 (zero).
+        #
+        {Credo.Check.Design.TagTODO, [exit_status: 0]},
+        {Credo.Check.Design.TagFIXME, []},
+
+        #
+        ## Readability Checks
+        #
+        {Credo.Check.Readability.AliasOrder, []},
+        {Credo.Check.Readability.FunctionNames, []},
+        {Credo.Check.Readability.LargeNumbers, []},
+        {Credo.Check.Readability.MaxLineLength, [priority: :low, max_length: 120]},
+        {Credo.Check.Readability.ModuleAttributeNames, []},
+        {Credo.Check.Readability.ModuleDoc, []},
+        {Credo.Check.Readability.ModuleNames, []},
+        {Credo.Check.Readability.ParenthesesInCondition, []},
+        {Credo.Check.Readability.ParenthesesOnZeroArityDefs, parens: true},
+        {Credo.Check.Readability.PredicateFunctionNames, []},
+        {Credo.Check.Readability.PreferImplicitTry, []},
+        {Credo.Check.Readability.RedundantBlankLines, []},
+        {Credo.Check.Readability.Semicolons, []},
+        {Credo.Check.Readability.SpaceAfterCommas, []},
+        {Credo.Check.Readability.StringSigils, []},
+        {Credo.Check.Readability.TrailingBlankLine, []},
+        {Credo.Check.Readability.TrailingWhiteSpace, []},
+        {Credo.Check.Readability.UnnecessaryAliasExpansion, []},
+        {Credo.Check.Readability.VariableNames, []},
+        {Credo.Check.Readability.WithSingleClause, false},
+
+        #
+        ## Refactoring Opportunities
+        #
+        {Credo.Check.Refactor.CondStatements, []},
+        {Credo.Check.Refactor.CyclomaticComplexity, []},
+        {Credo.Check.Refactor.FunctionArity, []},
+        {Credo.Check.Refactor.LongQuoteBlocks, []},
+        {Credo.Check.Refactor.MapInto, false},
+        {Credo.Check.Refactor.MatchInCondition, []},
+        {Credo.Check.Refactor.NegatedConditionsInUnless, []},
+        {Credo.Check.Refactor.NegatedConditionsWithElse, []},
+        {Credo.Check.Refactor.Nesting, []},
+        {Credo.Check.Refactor.UnlessWithElse, []},
+        {Credo.Check.Refactor.WithClauses, []},
+
+        #
+        ## Warnings
+        #
+        {Credo.Check.Warning.BoolOperationOnSameValues, []},
+        {Credo.Check.Warning.ExpensiveEmptyEnumCheck, []},
+        {Credo.Check.Warning.IExPry, []},
+        {Credo.Check.Warning.IoInspect, []},
+        {Credo.Check.Warning.LazyLogging, false},
+        {Credo.Check.Warning.MixEnv, []},
+        {Credo.Check.Warning.OperationOnSameValues, []},
+        {Credo.Check.Warning.OperationWithConstantResult, []},
+        {Credo.Check.Warning.RaiseInsideRescue, []},
+        {Credo.Check.Warning.UnusedEnumOperation, []},
+        {Credo.Check.Warning.UnusedFileOperation, []},
+        {Credo.Check.Warning.UnusedKeywordOperation, []},
+        {Credo.Check.Warning.UnusedListOperation, []},
+        {Credo.Check.Warning.UnusedPathOperation, []},
+        {Credo.Check.Warning.UnusedRegexOperation, []},
+        {Credo.Check.Warning.UnusedStringOperation, []},
+        {Credo.Check.Warning.UnusedTupleOperation, []},
+        {Credo.Check.Warning.UnsafeExec, []},
+
+        #
+        # Checks scheduled for next check update (opt-in for now, just replace `false` with `[]`)
+
+        #
+        # Controversial and experimental checks (opt-in, just replace `false` with `[]`)
+        #
+        {Credo.Check.Readability.StrictModuleLayout,
+         priority: :normal, order: ~w/shortdoc moduledoc behaviour use import require alias/a},
+        {Credo.Check.Consistency.MultiAliasImportRequireUse, false},
+        {Credo.Check.Consistency.UnusedVariableNames, []},
+        {Credo.Check.Design.DuplicatedCode, false},
+        {Credo.Check.Readability.AliasAs, false},
+        {Credo.Check.Readability.MultiAlias, false},
+        {Credo.Check.Readability.Specs, []},
+        {Credo.Check.Readability.SinglePipe, false},
+        {Credo.Check.Readability.WithCustomTaggedTuple, false},
+        {Credo.Check.Refactor.ABCSize, false},
+        {Credo.Check.Refactor.AppendSingleItem, false},
+        {Credo.Check.Refactor.DoubleBooleanNegation, false},
+        {Credo.Check.Refactor.ModuleDependencies, false},
+        {Credo.Check.Refactor.NegatedIsNil, false},
+        {Credo.Check.Refactor.PipeChainStart, false},
+        {Credo.Check.Refactor.VariableRebinding, false},
+        {Credo.Check.Warning.LeakyEnvironment, false},
+        {Credo.Check.Warning.MapGetUnsafePass, false},
+        {Credo.Check.Warning.UnsafeToAtom, false}
+
+        #
+        # Custom checks can be created using `mix credo.gen.check`.
+        #
+      ]
+    }
+  ]
+}

lib/membrane_opus/decoder.ex

@@ -13,8 +13,8 @@ defmodule Membrane.Opus.Decoder do
   @avg_opus_packet_size 960
 
   def_options sample_rate: [
-                spec: 8000 | 12000 | 16000 | 24000 | 48000,
-                default: 48000,
+                spec: 8_000 | 12_000 | 16_000 | 24_000 | 48_000,
+                default: 48_000,
                 description: """
                 Sample rate to decode at. Note: Opus is able to decode any stream
                 at any supported sample rate. 48 kHz is recommended. For details,

lib/membrane_opus/encoder.ex

@@ -141,18 +141,18 @@ defmodule Membrane.Opus.Encoder do
       :voip -> {:ok, 2048}
       :audio -> {:ok, 2049}
       :low_delay -> {:ok, 2051}
-      _ -> {:error, "Invalid application"}
+      _invalid -> {:error, "Invalid application"}
     end
   end
 
   defp validate_sample_rate(sample_rate) when sample_rate in @allowed_sample_rates do
     {:ok, sample_rate}
   end
 
-  defp validate_sample_rate(_), do: {:error, "Invalid sample rate"}
+  defp validate_sample_rate(_invalid_sr), do: {:error, "Invalid sample rate"}
 
   defp validate_channels(channels) when channels in @allowed_channels, do: {:ok, channels}
-  defp validate_channels(_), do: {:error, "Invalid channels"}
+  defp validate_channels(_invalid_channels), do: {:error, "Invalid channels"}
 
   defp frame_size(state) do
     # 20 milliseconds

lib/membrane_opus/parser.ex

@@ -14,8 +14,10 @@ defmodule Membrane.Opus.Parser do
   alias Membrane.{Buffer, Opus, RemoteStream}
   alias Membrane.Opus.Util
 
+  @type delimitation_t :: :delimit | :undelimit | :keep
+
   def_options delimitation: [
-                spec: Delimitation.delimitation_t(),
+                spec: delimitation_t(),
                 default: :keep,
                 description: """
                 If input is delimitted? (as indicated by the `self_delimiting?`
@@ -76,7 +78,7 @@ defmodule Membrane.Opus.Parser do
   end
 
   @impl true
-  def handle_process(:input, %Buffer{payload: data}, _ctx, state) do
+  def handle_process(:input, %Buffer{payload: data}, ctx, state) do
     {delimitation_processor, self_delimiting?} =
       Delimitation.get_processor(state.delimitation, state.input_delimitted?)
 
@@ -87,19 +89,23 @@ defmodule Membrane.Opus.Parser do
            delimitation_processor
          ) do
       {:ok, buffer, pts, packets, channels} ->
+        caps = %Opus{
+          self_delimiting?: self_delimiting?,
+          channels: channels
+        }
+
+        packets_len = length(packets)
+
         packet_actions =
-          if length(packets) > 0 do
-            [
-              caps:
-                {:output,
-                 %Opus{
-                   channels: channels,
-                   self_delimiting?: self_delimiting?
-                 }},
-              buffer: {:output, packets}
-            ]
-          else
-            []
+          cond do
+            packets_len > 0 and caps != ctx.pads.output.caps ->
+              [caps: {:output, caps}, buffer: {:output, packets}]
+
+            packets_len > 0 ->
+              [buffer: {:output, packets}]
+
+            true ->
+              []
           end
 
         {{:ok, packet_actions ++ [redemand: :output]}, %{state | buffer: buffer, pts: pts}}
@@ -167,7 +173,7 @@ defmodule Membrane.Opus.Parser do
       <<raw_packet::binary-size(expected_packet_size), rest::binary>> ->
         {:ok, raw_packet, rest}
 
-      _ ->
+      _otherwise ->
         {:error, :cont}
     end
   end

lib/membrane_opus/parser/delimitation.ex

@@ -2,11 +2,10 @@ defmodule Membrane.Opus.Parser.Delimitation do
   @moduledoc false
   # Helper module for delimiting or undelimiting packets
 
-  @type delimitation_t :: :delimit | :undelimit | :keep
   @type processor_t :: __MODULE__.Undelimiter | __MODULE__.Delimiter | __MODULE__.Keeper
 
   @spec get_processor(
-          delimitation :: delimitation_t(),
+          delimitation :: Membrane.Parser.Opus.delimitation_t(),
           self_delimiting? :: boolean
         ) ::
           {processor :: processor_t, self_delimiting? :: boolean}

lib/membrane_opus/parser/frame_lengths.ex

@@ -115,7 +115,7 @@ defmodule Membrane.Opus.Parser.FrameLengths do
         vbr_flag == 1
       )
     else
-      _ ->
+      _otherwise ->
         {:error, :cont}
     end
   end

lib/membrane_opus/util.ex

@@ -26,6 +26,9 @@ defmodule Membrane.Opus.Util do
            bandwidth :: :narrow | :medium | :wide | :super_wide | :full,
            frame_duration :: Membrane.Time.non_neg_t()}
           | :error
+  # Credo CC check thinks that this function is super complex, but for an actual human it isn't.
+  # Therefore, it makes sense to disable the check for this function
+  # credo:disable-for-next-line
   def parse_configuration(configuration_number) do
     case configuration_number do
       0 -> {:ok, :silk, :narrow, 10 |> milliseconds()}
@@ -60,7 +63,7 @@ defmodule Membrane.Opus.Util do
       29 -> {:ok, :celt, :full, 5 |> milliseconds()}
       30 -> {:ok, :celt, :full, 10 |> milliseconds()}
       31 -> {:ok, :celt, :full, 20 |> milliseconds()}
-      _ -> :error
+      _otherwise -> :error
     end
   end
 end

mix.exs

@@ -42,7 +42,8 @@ defmodule Membrane.Opus.Plugin.Mixfile do
       {:unifex, "~> 0.7.0"},
       {:membrane_common_c, "~> 0.10.0"},
       {:dialyxir, "~> 1.0.0", only: :dev, runtime: false},
-      {:ex_doc, "~> 0.23", only: :dev},
+      {:ex_doc, "~> 0.23", only: :dev, runtime: false},
+      {:credo, ">= 0.0.0", only: :dev, runtime: false},
       {:membrane_file_plugin, "~> 0.7.0", only: :test}
     ]
   end