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

Showing bitrate in video details #714

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Showing bitrate in video details #714

merged 2 commits into from
Dec 14, 2021

Conversation

jozefkopcak
Copy link
Contributor

This is what I ended up with, I hope it satisfies :)

Copy link
Owner

@whyboris whyboris left a comment

Choose a reason for hiding this comment

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

Looks great 🎉
Thank you for your contribution 🙇 🤝 😁

@@ -33,7 +33,9 @@
| {{ video.width }} x {{ video.height }}

| {{ video.fileSizeDisplay }}


| {{ video.bitrate + ' mb/s' }}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm happy to accept as is. But currently if bitrate property is absent (as it would be for old Hubs), there would be an empty display. I recommend something like the line below, for example:
{{ video.bitrate ? '| ' + video.bitrate + ' mb/s' : '' }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will always be a bitrate property, because it's calculated from videoSize and duration, which are present in every video there is (if I understand correctly).

Copy link
Owner

@whyboris whyboris Dec 14, 2021

Choose a reason for hiding this comment

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

Oh you're right! I'm sorry, in my mind, before I saw your code, I was thinking you were grabbing the bitrate from what FFmpeg was returning (when initially scanning the video), and then I commented based on my guess about your code rather than your code 😓

I like your implementation rather than what I was thinking, since it provides previously-scanned videos with a bitrate estimate!

I'll merge this in soon 👏

@whyboris
Copy link
Owner

Closes issue #712 🙇 🤝 😁 Thank you!

@whyboris whyboris merged commit a926bf4 into whyboris:main Dec 14, 2021
@whyboris
Copy link
Owner

whyboris commented Dec 14, 2021

Thank you @jozefkopcak for the lovely contribution to Video Hub App 🙇

I might tweak it a bit before release (I think the standard way people think about bitrate is using bits, so I just need to multiply by 8 in your formula 😅).

To this day I get frustrated between "megabits" and "megabytes" (and in the Issue asked for the wrong thing I think) - and while I like seeing how many megabytes are used per second, I think most video people expect megabits 🤷 (unsure why - perhaps because this measure was easier to think about when streaming over the internet, which for some annoying reason was in bits not in bytes :trollface: )

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