Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add media duration to lpms_get_codec_info for GetCodecInfo #407
Add media duration to lpms_get_codec_info for GetCodecInfo #407
Changes from 17 commits
eef214e
1fcc47e
904f309
6a2b014
0e84f60
2b723a4
b601475
bad8498
146d718
75bd67c
e6389f2
f05997e
f0eda1a
f3c8917
d025d0a
6579e6e
67db771
cc58010
8cdef44
a8f19c4
d99d32b
f2edd37
1fd0f5c
afaf25d
56c2328
0e12477
c85f817
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, it would be nice to also add checks for expected fps/duration values from ffprobe - helps ensure that our own calculations are correct
also just checking, which one of these triggers the no-duration case that requires manual calculations? (that should also surface via ffprobe, eg a duration of "N/A")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be with webm and mp3 files.
ffprobe -show_streams test.webm | grep 'duration='
gives N/A for duration. I've added webm to the test, it is working with the latest changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look and the duration is there for the top-level container (
ic->duration
) for both webm and mp3 files. You can also see this in the top-level ffprobe output, orffprobe -show_format
. Stream duration is indeed empty (which is the equivalent withffprobe -show_streams
) but we aren't using that here.If we need to introduce a code path to manually calculate the duration then I would prefer to find a test case which can exercise that.
Also please add the ffprobe outputs to the test case as well, because this validates the assumptions we are testing against, and allows us to spot areas where we diverge from ffmpeg's own behavior. For example:
this example is shortened to 1 second; it doesn't really need to be very long for the purpose of this test case and speeds up test execution since its encoding video which is expensive
BTW the durations you have in these test cases seem very round - do you care about the fractional part of the second at all? If not, then maybe make it an integer type instead of floating point. Otherwise you'll need to cast to double when calculating the duration, eg
out->dur = (double)ic->duration / AV_TIME_BASE
;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the ffprobe commands, shortened all the videos to 2 seconds. The int64 change is working well so I removed
calculate_stream_duration
. I am happy with rounding numbers for nowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checking (no pun intended) ... anything less than 1 (eg, 0.99) will be rounded down to 0, 1.99 becomes 1, etc. That seems pretty significant (eg, how do you distinguish between "very short" and "zero duration") but if is still OK for your purposes, then 👍
Otherwise, I believe we can still match the values calculated by ffprobe - our equivalent would be
out->dur = ic->duration * av_q2d(AV_TIME_BASE_Q)
... mathematically the same as the old code, but I don't know if the computed precision would differ (and it would only come into play for very large durations, anyhow)