Skip to content

Conversation

@varsill
Copy link
Collaborator

@varsill varsill commented Nov 6, 2025

@varsill varsill changed the base branch from master to varsill/support_tden November 6, 2025 14:53
@varsill varsill marked this pull request as ready for review November 18, 2025 11:30
Comment on lines 83 to 90
case Map.get(demuxing_engine.packets_map, track_id) do
[packet | rest] ->
demuxing_engine = put_in(demuxing_engine.packets_map[track_id], rest)
{[packet], demuxing_engine}

_other ->
{[], demuxing_engine}
end
Copy link
Member

@mat-hek mat-hek Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case Map.get(demuxing_engine.packets_map, track_id) do
[packet | rest] ->
demuxing_engine = put_in(demuxing_engine.packets_map[track_id], rest)
{[packet], demuxing_engine}
_other ->
{[], demuxing_engine}
end
get_and_update_in(demuxing_engine.packets_map[track_id], fn
[packet | rest] -> {[packet], rest}
other -> {[], other}
end

I'd also move this function below pop_chunk

Copy link
Collaborator Author

@varsill varsill Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can use get_and_update_in here, as if we were to call take_packets/2 for a track_id which is not yet in packets_map, it would add an empty entry under this track_id key.
(it might happen when track_id is resolved based on PMT, but no PES packet with given track_id has yet arrived)


packets_map =
Enum.reduce(new_packets, demuxing_engine.packets_map, fn new_packet, packets_map ->
Map.update(packets_map, new_packet.pid, [new_packet], &(&1 ++ [new_packet]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use Qex

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, done

Comment on lines 85 to 86
{maybe_tden_tag, demuxing_engine} = maybe_read_tden_tag(demuxing_engine, packet.payload.pts)
tden_tag = maybe_tden_tag || demuxing_engine.last_tden_tag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me its unclear if maybe_read_tden_tag(demuxing_engine, packet.payload.pts)

  • returns tden tag
  • or sets it in demuxing_engine.last_tden_tag
  • or both

because this function returns demuxing_engine as well as maybe_tden_tag. What about making it return only demuxing_engine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 112 to 119
case Map.get(demuxing_engine.packets_map, track_id) do
[packet | rest] ->
demuxing_engine = put_in(demuxing_engine.packets_map[track_id], rest)
{[packet], demuxing_engine}

_other ->
{[], demuxing_engine}
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want, you can use get_and_update_in - it will simplify this piece of code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would change the behaviour, as described here: #17 (comment)

@varsill varsill requested a review from FelonEkonom November 19, 2025 11:19
Comment on lines +110 to +115
with {:ok, packets} <- Map.fetch(demuxing_engine.packets_map, track_id),
{{:value, packet}, rest} <- Qex.pop(packets) do
demuxing_engine = put_in(demuxing_engine.packets_map[track_id], rest)
{[packet], demuxing_engine}
else
_other -> {[], demuxing_engine}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

Copy link
Member

@FelonEkonom FelonEkonom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@varsill varsill merged commit e84bebb into varsill/support_tden Nov 19, 2025
3 checks passed
@varsill varsill mentioned this pull request Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update ex_hls dependency to mpeg_ts v3

4 participants