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

--read-length CLI option + some code suggestions #11

Merged
merged 9 commits into from
Aug 16, 2023

Conversation

GabrielChanzy
Copy link
Contributor

Hello,
Thanks so much for making your project available, I've really benefited from it :)
I've added an additional --read-length option for my use case (getting stats for live RTSP streams). My edits have been heavily influenced by this other forked repo. They seemed to stop maintaining it so thought to fork your project & initiate a pull request. (The gist of it is the --read_intervals option of ffprobe)

The other commits are small edits I made in the code, the last one being a bug fix for the new --read-length option. Hopefully the commit message is clear enough; let me know if you require clarifications.

Thanks for taking the time to go through this. This is my first time trying out forks & pull requests so please let me know if I could have done something better! I am pretty unfamiliar with FFmpeg & python as well, so let me know if I misunderstood something :)

- duration is rounded at calculation & when creating BitrateStatsSummary
- removed verbose input parameter
- added debug level log
- calculates pts from next packet if value is missing
@slhck
Copy link
Owner

slhck commented Aug 1, 2023

Thanks for the suggestion. I can see how this is useful!

Some thoughts: It imposes some restrictions on the value of read_intervals. While you might be able to specify 10%+20,01:30%01:45 in ffprobe, here we just allow a single number. I guess this is fine for most applications though.

I will leave some more comments in the code.

Comment on lines 202 to 203
# "-analyzeduration",
# str(self.chunk_size*1000000),
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this if not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my tests, it seems unnecessary; I copied it over from unleashlive's repo. I'll remove it.

As a side note, do you understand what the -analyzeduration option does? From what I read, it seems to specify the duration that ffprobe's probe attempts to read the input file's metadata/headers. Is that right?

ffmpeg_bitrate_stats/bitrate_stats.py Outdated Show resolved Hide resolved
@GabrielChanzy
Copy link
Contributor Author

I've tested the latest commit with a video I ripped off youtube (it's 6+ mins long).
image

Some thoughts: It imposes some restrictions on the value of read_intervals. While you might be able to specify 10%+20,01:30%01:45 in ffprobe, here we just allow a single number. I guess this is fine for most applications though.

I've changed the --read-length option to --read-start & --read-end. Only takes in seconds for now. For read end, it will be relative duration rather than absolute (eg -rs 1 -re 2 means start from 1 second onwards and read for 2 seconds).

Here is how the help looks like
image

@slhck
Copy link
Owner

slhck commented Aug 4, 2023

That makes sense, running the tests again.

@slhck
Copy link
Owner

slhck commented Aug 4, 2023

Actually, looking at it again, I will change the parameter to be --read-duration rather than --read-end, because it's not the end time that is specified. Otherwise it's fine to merge, will do so later, thanks for the PR!

@slhck
Copy link
Owner

slhck commented Aug 4, 2023

I had to fix some types using mypy. Now I think it has no issues anymore, I pushed a new commit to this PR. Can you please check though if the functionality works as intended? When I specify:

python3 -m ffmpeg_bitrate_stats test/test.mp4 -rs 0.5 -v

I would expect the output to contain 12 frames only (the full 1 second file has 25 frames), but I still get 25?

@slhck
Copy link
Owner

slhck commented Aug 9, 2023

Did you get a chance to check the functionality? I'd like to merge it but I'm not sure if this does what it is supposed to do.

@GabrielChanzy
Copy link
Contributor Author

Hi sorry, I didn't have the time to come back to this PR this week. I should be able to get back to you next Tue, let you know then :)

- occurs with --read-duration)
@GabrielChanzy
Copy link
Contributor Author

GabrielChanzy commented Aug 14, 2023

Had some issues (when using -rd) with the 1st packet's duration value, added some code for that.


When I specify:

python3 -m ffmpeg_bitrate_stats test/test.mp4 -rs 0.5 -v

I would expect the output to contain 12 frames only (the full 1 second file has 25 frames), but I still get 25?

Seems like the --read-intervals option seeks to the nearest second.
image

I happen to do some tests with the HH:MM:SS format, thought to share it with you.
image

from the documentation
image


I should be good after this commit, let me know if there is anything else I can do. Otherwise, pleasure working with you! 😄

- added check for 1st packet's duration type
@slhck
Copy link
Owner

slhck commented Aug 16, 2023

Ah yes, it says in the docs:

Note that seeking is not accurate, thus the actual interval start point may be different from the specified position. Also, when an interval duration is specified, the absolute end time will be computed by adding the duration to the interval start point found by seeking the file, rather than to the specified start value.

@slhck slhck merged commit 7ade99d into slhck:master Aug 16, 2023
@slhck
Copy link
Owner

slhck commented Aug 16, 2023

Released in v1.1.0, thanks for your contribution!

@slhck
Copy link
Owner

slhck commented Aug 16, 2023

@all-contributors Please add @GabrielChanzy for code

@allcontributors
Copy link
Contributor

@slhck

I've put up a pull request to add @GabrielChanzy! 🎉

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.

2 participants