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

Filters #289

Merged
merged 13 commits into from Oct 1, 2021
Merged

Filters #289

merged 13 commits into from Oct 1, 2021

Conversation

multiflexi
Copy link
Contributor

Description

Improved scaling - with help of @slhck, improved filter string creation, added automatic deinterlacing for interlaced content.

Before
Screenshot from 2021-09-10 10-26-24
After deinterlacing
Screenshot from 2021-09-10 10-26-48

multiflexi and others added 11 commits July 9, 2021 13:38
Keep original framerate up to 60fps, halve any framerate above 60fps. Because of "video_frame_rate": Fraction(video_info["r_frame_rate"]), it does not work, when float used, the video is encoded but framerate suffers from rounding error.
Keep original framerate up to 60fps, halve any framerate above 60fps. Because of "video_frame_rate": Fraction(video_info["r_frame_rate"]), it does not work, when float used, the video is encoded but framerate suffers from rounding error.
files/helpers.py Outdated
@@ -523,6 +530,27 @@ def get_base_ffmpeg_command(
if target_fps < 1:
target_fps = 1

filters = []

if interlaced == True:
Copy link
Contributor

Choose a reason for hiding this comment

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

more pythonic

if interlaced:
    filters.append("yadif")

@mgogoulos
Copy link
Contributor

Thanks for this PR @multiflexi and @slhck !

@multiflexi , please follow instructions on https://github.com/mediacms-io/mediacms/blob/main/docs/developers_docs.md#4-how-to-contribute in order to install pre-commit and have it format the code - so that formatting tests pass

I'm working on some video upload tests on another branch so will be more confident to merge them first and after that this can be merged as well

@mgogoulos
Copy link
Contributor

Hi @multiflexi , can you follow instructions to install locally pre-commit and have it format the code as the github action tests? I can do that after I merge your PR but it's better if you do it.

I've also added the new video uploads tests and am more confident to merge.

@slhck can you also have a check on this PR and approve, or suggest we perform any change?

Thanks and regards

@slhck
Copy link
Contributor

slhck commented Sep 23, 2021

It follows what we had talked about in a short call. Consistent style should be followed, as you wrote, but otherwise LGTM!

@multiflexi
Copy link
Contributor Author

Hi, sorry it took so long, I was too busy. Hope this is better.

@mgogoulos
Copy link
Contributor

I've just tested this PR on a video and I see that transcoded videos are slightly bigger in size - example check these two screenshots, with the current ffmpeg settings and the ones introduced here.

Does this increase in size make sense @multiflexi @slhck?

with this PR
PR_289
with current settings
current

@slhck
Copy link
Contributor

slhck commented Sep 27, 2021

Was this a portrait video or one that had interlacing? Otherwise, I don't think it should be affecting the size too much. The variations you describe – in the order of a few percent – are not significant IMO.

Is the size difference purely due to the changes in this branch, or have the videos been previously encoded by another ffmpeg version maybe?

@multiflexi
Copy link
Contributor Author

What file did you use?

@mgogoulos
Copy link
Contributor

I've used this file: https://demo.mediacms.io/view?m=SUHVrcpfc

the only difference should be the changes in this branch, however I'm not 100% sure whether both environments have used the same ffmpeg version which complicates things I guess. More important, if you think that the increase is not something to worry, we should be ready to merge

@multiflexi
Copy link
Contributor Author

The change in file size is nothing to worry about. I tried it on my instance, and the size is different as well. It is smaller than the files on the demo server and somewhere between filesizes on your screenshots. We could further improve quality/smaller file size with slow/veryslow settings, but it increases the encoding time. It depends on what user priority is.

@mgogoulos
Copy link
Contributor

Ok I think we are good to go in this case!

@mgogoulos mgogoulos merged commit 28031f0 into mediacms-io:main Oct 1, 2021
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

3 participants