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

store ctts table #115

Merged
merged 6 commits into from
Jul 29, 2024
Merged

Conversation

gBillal
Copy link
Contributor

@gBillal gBillal commented Jul 8, 2024

In this PR:

  • We store the ctts table on muxing.
  • Updated the first sample pts on demuxing, the first sample's dts is 0, however it's not necessary the same for the pts.
  • Updated the test files (there's rounding errors due to conversion between the timescales, so the files are not the same after setting the add_dts_offset to false)

@gBillal gBillal requested a review from mat-hek as a code owner July 8, 2024 23:10
@mat-hek
Copy link
Member

mat-hek commented Jul 9, 2024

Hi @gBillal, thanks for the PR ;)

Updated the first sample pts on demuxing, the first sample's dts is 0,

I'm unsure if this is true. If the video PTS starts from 0, the DTS must start from a negative value, so that PTS is always greater than DTS, and that is required by, for example, FFmpeg libs.

@gBillal
Copy link
Contributor Author

gBillal commented Jul 9, 2024

Hi @mat-hek
I'm not sure that I'm following you, that change to the first sample will always satisfy that condition (pts being always >= dts).

@mat-hek
Copy link
Member

mat-hek commented Jul 9, 2024

Hmm, I may have misunderstood it 🤔 let's approach it another way: why is this change needed: https://github.com/membraneframework/membrane_mp4_plugin/pull/115/files#diff-2d67f6484e673f781de79faf98a69c97f7bd56018e49d685b52b6678751c4fb0R53 ?

@gBillal
Copy link
Contributor Author

gBillal commented Jul 9, 2024

Ah, you're talking about that. I did add that to check that the ctts table is not included if the diff between pts and dts is 0 for all samples. I was sure it'll be 0, because the streams doesn't include B-frames. However I didn't set the add_dts_offset to false for the hevc stream because it includes B-frames.

In other words, that change doesn't add much anyway, I can revert it to the old one.

@mat-hek
Copy link
Member

mat-hek commented Jul 10, 2024

@gBillal gotcha, so I'd revert that and add another test to check if ctts is absent when add_dts_offset is set to false. Otherwise looks good ;)

@mat-hek mat-hek requested a review from varsill July 10, 2024 14:58
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

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

Seems good, thanks!

test/membrane_mp4/muxer/isom/integration_test.exs Outdated Show resolved Hide resolved
@gBillal gBillal requested a review from varsill July 11, 2024 15:37
@mat-hek
Copy link
Member

mat-hek commented Jul 12, 2024

@gBillal please make sure that formatter, credo and dialyzer pass ;)

@gBillal
Copy link
Contributor Author

gBillal commented Jul 12, 2024

@mat-hek what's the command you run for credo. mix credo works fine on my side

@mat-hek
Copy link
Member

mat-hek commented Jul 12, 2024

This is because of Elixir version and credo incompatibility. Please run mix deps.update credo or even better mix deps.update --all and it should be fine ;)

@varsill
Copy link
Contributor

varsill commented Jul 23, 2024

I believe @mat-hek 's approve is required before merging as he is the code owner of the plugin

@darthez darthez assigned darthez and mat-hek and unassigned darthez Jul 24, 2024
@mat-hek mat-hek merged commit 22e7ae8 into membraneframework:master Jul 29, 2024
@gBillal gBillal deleted the store-ctts-table branch July 29, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants