Handle TDEN frame in timed ID3v2 stream #13
Conversation
…aylists and segments. Fallback to MPEG-TS demuxer
…with nil ID3 chunks timestamps. Add test
| {:mpeg_ts, | ||
| github: "membraneframework-labs/kim_mpeg_ts", | ||
| branch: "varsill/fix_pes_optional_header_resolving"}, |
There was a problem hiding this comment.
We either need to wait for backport of my bugfix on mpeg_ts v2 or update our dependency to mpeg_ts v3
…e the playback starts as fast as possible in ultra low latency mode
| nil -> {nil, demuxer} | ||
| {[], updated_demuxer} -> {nil, updated_demuxer} | ||
| false -> {nil, demuxer} |
There was a problem hiding this comment.
This can easily get messed up so that a wrong clause matches. Either use withl or reshape it to be less ambiguous
| with {pos, len} <- :binary.match(payload, "TDEN"), | ||
| trailing_bytes <- :binary.part(payload, pos + len, byte_size(payload) - (pos + len)), | ||
| <<size::integer-size(4)-unit(8), _flags::16, rest::binary>> <- trailing_bytes, | ||
| <<_3, text::binary-size(size - 2), 0, _rest::binary>> <- rest do |
There was a problem hiding this comment.
| with {pos, len} <- :binary.match(payload, "TDEN"), | |
| trailing_bytes <- :binary.part(payload, pos + len, byte_size(payload) - (pos + len)), | |
| <<size::integer-size(4)-unit(8), _flags::16, rest::binary>> <- trailing_bytes, | |
| <<_3, text::binary-size(size - 2), 0, _rest::binary>> <- rest do | |
| with {pos, _len} <- :binary.match(payload, "TDEN"), | |
| <<_skip::binary-size(pos), "TDEN", tden::binary>> <- payload, | |
| <<size::integer-size(4)-unit(8), _flags::16, _3, text::binary-size(size - 2), 0, _rest::binary>> <- tden do |
btw, what is _3?
There was a problem hiding this comment.
It's a flag indicating encoding of the timestamps string (3 stands for UTF-8)
I think it's good enough to support just UTF-8 encoded strings (so I will make it match for exact value of 3 in this binary) - WDYT?
test/client_test.exs
Outdated
| &(&1.metadata.tden_tag != nil and | ||
| &1.media_type == | ||
| :video) | ||
| ) |
There was a problem hiding this comment.
looks weird, if can't be fixed let's change to fn
There was a problem hiding this comment.
perhaps it's because of formatting? These are just two conditions merged with AND:
&(&1.metadata.tden_tag != nil and &1.media_type == :video)
There was a problem hiding this comment.
I only meant the formatting is weird
Co-authored-by: Mateusz Front <mfront3@gmail.com>
| defp parse_tden_tag(payload) do | ||
| with {pos, _len} <- :binary.match(payload, "TDEN"), | ||
| <<_skip::binary-size(pos), "TDEN", tden::binary>> <- payload, | ||
| <<size::integer-size(4)-unit(8), _flags::16, _3, text::binary-size(size - 2), 0, |
There was a problem hiding this comment.
I think we agreed we need to
match on '3'?
| <<size::integer-size(4)-unit(8), _flags::16, _3, text::binary-size(size - 2), 0, | ||
| _rest::binary>> <- tden do | ||
| <<size::integer-size(4)-unit(8), _flags::16, ^encoding::8, text::binary-size(size - 2), | ||
| 0::8, _rest::binary>> <- tden do |
There was a problem hiding this comment.
NIT: 8 bits is the default for an integer, so 0::8 can become 0
There was a problem hiding this comment.
Sure, but as discussed - I think it's easier for me to read it with these ::8 as they are surrounded by ::16 etc. :D
Required by membraneframework/membrane_core#1009