Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix chunk offsets #110

Merged
merged 11 commits into from
Apr 23, 2024
Merged

Fix chunk offsets #110

merged 11 commits into from
Apr 23, 2024

Conversation

varsill
Copy link
Contributor

@varsill varsill commented Apr 22, 2024

No description provided.

…value per each sample. Consume the mdat iterator while reading data from mdat
…of mdat data. Calculate start of mdat based on this function.
@varsill varsill requested a review from mat-hek as a code owner April 22, 2024 10:06
@varsill varsill changed the base branch from master to add_version_1_boxes April 22, 2024 10:06
Comment on lines 68 to 72
# data =
# case data do
# <<0, 0, 0, 0, 0, 0, 0, 0, rest::binary>> -> rest
# _other -> data
# end
Copy link
Member

Choose a reason for hiding this comment

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

Is it left just in case? :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed ;)

Comment on lines 225 to 226
sample =
Map.put(sample, :track_id, track_id) |> Map.put(:chunk_offset, chunk_offset)
Copy link
Member

Choose a reason for hiding this comment

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

We could accumulate the offset of each chunk here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -141,12 +142,15 @@ defmodule Membrane.MP4.Demuxer.ISOM do
fsm_state: :samples_info_present_and_all_pads_connected
} = state
) do
{samples, rest, samples_info} =
SamplesInfo.get_samples(state.samples_info, state.partial <> buffer.payload)
{samples, {rest, rest_iterator}, samples_info} =
Copy link
Member

Choose a reason for hiding this comment

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

The rest_iterator could be initialized in SamplesInfo.get_samples_info and kept in samples_info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@varsill varsill requested a review from mat-hek April 23, 2024 13:07
Comment on lines 211 to 213
{state.mdat_beginning || get_mdat_header_beginning(state.boxes),
state.mdat_header_size || maybe_header[:header_size] || state.boxes[:mdat].header_size,
state.mdat_size || maybe_header[:content_size] || state.boxes[:mdat].size}
Copy link
Member

Choose a reason for hiding this comment

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

let's update the state here

Copy link
Contributor 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 74 to 75
if to_skip + size <= byte_size(data) do
<<_to_skip::binary-size(to_skip), payload::binary-size(size), rest::binary>> = data
Copy link
Member

Choose a reason for hiding this comment

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

It would look better as a case

Suggested change
if to_skip + size <= byte_size(data) do
<<_to_skip::binary-size(to_skip), payload::binary-size(size), rest::binary>> = data
case data do
<<_to_skip::binary-size(to_skip), payload::binary-size(size), rest::binary>> ->

Copy link
Contributor 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 91 to 93
[
buffer | buffers
]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[
buffer | buffers
]
[buffer | buffers]

Copy link
Contributor 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 217 to 219
Map.put(sample, :track_id, track_id)
|> Map.put(:chunk_offset, chunk_offset)
|> Map.put(:sample_offset, sample_offset)
Copy link
Member

Choose a reason for hiding this comment

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

let's use Map.merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@varsill varsill merged commit 160794e into add_version_1_boxes Apr 23, 2024
3 checks passed
@varsill varsill deleted the fix_chunk_offsets branch April 23, 2024 14:47
varsill added a commit that referenced this pull request Apr 25, 2024
* Add support for version1 boxes. Add parsing of co64 box

* Fix chunk offsets (#110)

* bump to v0.34.2
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.

None yet

2 participants