Feature/payload types #82

bblaszkow06 · 2018-08-06T12:58:16Z

Closes #68

mat-hek · 2018-08-07T09:14:22Z

lib/membrane/buffer.ex

  @type metadata_t :: map

  @type t :: %Buffer{
-          payload: payload_t,
+          payload: Payload.t(),
+          type: :binary | :shm | :pointer,


Why is this field necessary? I don't think Core should be aware of all payload types. Also name type is a bit misleading, because it determines the type of payload, not type of buffer. Maybe it could be replaced with type function in Payload protocol, returning atom.

mat-hek · 2018-08-07T09:31:59Z

lib/membrane/payload.ex

+  @moduledoc """
+  """
+
+  @type t :: struct()


This protocol can be implemented for anything, and it is already for binary, so I would say any

lib/membrane/payload.ex

+  def to_binary(payload)
+end
+
+defimpl Membrane.Payload, for: BitString do


lib/membrane/payload.ex

@@ -0,0 +1,50 @@
+defprotocol Membrane.Payload do
+  @moduledoc """


mat-hek · 2018-08-07T09:46:14Z

lib/membrane/payload.ex

+  end
+
+  @spec split_at(binary(), non_neg_integer) :: {binary(), binary()}
+  def split_at(data, 0) do


This is already covered by the last clause. I am also not sure if we should require supporting this in protocol, as we do not use this, and implementing for other payloads might be problematic.

mat-hek · 2018-08-07T09:58:30Z

lib/membrane/payload.ex

+  @doc """
+  Splits the payload at given position (1st part has the size equal to `at_pos` argument)
+  """
+  @spec split_at(payload :: t(), at_pos :: non_neg_integer()) :: {t(), t()}


I would change at_pos type to pos_integer

mat-hek · 2018-08-08T14:45:26Z

lib/membrane/payload.ex

  """
  @spec size(payload :: t()) :: non_neg_integer()
  def size(payload)

  @doc """
  Splits the payload at given position (1st part has the size equal to `at_pos` argument)
+
+  `at_pos` has to be greater than 0 and smaller than the size of payload. This guarantees


It sounds like a user would have to guarantee that at_pos is valid. I would rather write "at_pos is always greater than 0..."

This is still not resolved

mat-hek · 2018-08-08T14:47:23Z

lib/membrane/payload.ex

+  `at_pos` has to be greater than 0 and smaller than the size of payload. This guarantees
+  returned payloads are never empty.
+
+  When such conditions are not met, the function should raise.


I would remove this sentence, as it is responsibility of the core to invoke these functions with proper arguments. Additional checks might be performed to be sure. Going current way, we should also write that function should raise e.g. if a string is passed instead of integer.

mat-hek · 2018-08-08T14:52:26Z

lib/membrane/payload.ex

-  @spec split_at(payload :: t(), at_pos :: non_neg_integer()) :: {t(), t()}
-  def split_at(payload, at_pos)
+  @spec split_at!(payload :: t(), at_pos :: pos_integer()) :: {t(), t()}
+  def split_at!(payload, at_pos)


I think this exclamation is unnecessary. Specificaton of the function does not impose that any errors may happen

lib/membrane/payload.ex

+  Returns an atom describing type of the payload.
+  """
+  @spec type(t()) :: atom()
+  def type(payload)


mat-hek · 2018-08-10T16:28:25Z

lib/membrane/payload.ex

+  @doc """
+  Splits the payload at given position (1st part has the size equal to `at_pos` argument)
+
+  `at_pos` has to be greater than 0 and smaller than the size of payload. This guarantees


It sounds as if a user would have to guarantee that at_pos is valid. I would rather write "at_pos is always greater than 0..."

bblaszkow06 added 4 commits August 3, 2018 12:55

Define Payload as protocol

a0d6f2c

WIP

b554546

Update espec and remove invalid fun from Payload protocol

883a6af

Avoid wrapping binary payload in a struct

ed63b38

bblaszkow06 requested review from mtsznowak and mat-hek August 6, 2018 12:58

mat-hek requested changes Aug 7, 2018

View reviewed changes

Docs fixes and cleanup Buffer definition

d596dcd

bblaszkow06 force-pushed the feature/payload-types branch 5 times, most recently from 5de962e to 4d71e4f Compare August 8, 2018 07:09

Don't allow empty payload as split_at result

78d5a50

bblaszkow06 force-pushed the feature/payload-types branch from 4d71e4f to 78d5a50 Compare August 8, 2018 07:20

bblaszkow06 requested a review from mat-hek August 8, 2018 08:33

mat-hek requested changes Aug 8, 2018

View reviewed changes

Tiny rename

e00facf

bblaszkow06 force-pushed the feature/payload-types branch from 95d7e5e to e00facf Compare August 10, 2018 13:25

bblaszkow06 requested a review from mat-hek August 10, 2018 13:50

mat-hek reviewed Aug 10, 2018

View reviewed changes

Small fix in docs

f26227d

bblaszkow06 merged commit 4503162 into master Aug 13, 2018

bblaszkow06 deleted the feature/payload-types branch August 13, 2018 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/payload types #82

Feature/payload types #82

bblaszkow06 commented Aug 6, 2018

mat-hek Aug 7, 2018

mat-hek Aug 7, 2018

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mat-hek Aug 7, 2018

mat-hek Aug 7, 2018

mat-hek Aug 8, 2018

mat-hek Aug 10, 2018

mat-hek Aug 8, 2018

mat-hek Aug 8, 2018

This comment was marked as resolved.

mat-hek Aug 10, 2018 •

edited

		@@ -0,0 +1,50 @@
		defprotocol Membrane.Payload do
		@moduledoc """

Feature/payload types #82

Feature/payload types #82

Conversation

bblaszkow06 commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

This comment was marked as resolved.

mat-hek Aug 10, 2018 • edited

Choose a reason for hiding this comment

mat-hek Aug 10, 2018 •

edited