JuliaIO / VideoIO.jl

Reading and writing of video files in Julia via ffmpeg
https://juliaio.github.io/VideoIO.jl/stable
Other
126 stars 53 forks source link

Encoded videos have few, if any, keyframes #273

Closed galenlynch closed 3 years ago

galenlynch commented 3 years ago

I noticed when tinkering around with the seek code in VideoIO that it is hard to seek into videos that were made by VideoIO. I have also noticed that videos that I make with VideoIO look terrible in google previews if I upload them to dropbox to share with my collaborators. While looking at the encoding code, I noticed that unlike in the FFMPEG encoding example on which VideoIO's encoding seems to based on, the gop_size and max_b_frame field of the AVCodecContext struct are not set. Inspecting the value of gop_size of the struct if it is not set shows that it is -1, and the FFMPEG documentation does not mention this as a default. By running the following command on resulting videos, it shows that in fact all the pict_types of the frames appear to be P frames (predicted frames):

ffprobe -show_frames ~/video_under_test.mp4 | grep pict_type

This seems unusual and is probably not a good default choice. FWIW, the ffmpeg binary sets gop_size to 12 by default.

IanButterworth commented 3 years ago

Very interesting. I've only ever used VideoIO in the wild with crf = 0, which I would assume doesn't suffer this issue.

I could write this on every issue, but just wanted to say a big thank you for all your work. It's very much appreciated

galenlynch commented 3 years ago

No problem! I'm using it for my research right now so definitely in my interest that it works like I think it should!

Not sure about the crf=0 thing, I will check it out. There is still compression, and likely predicted frames, even if it is lossless.

IanButterworth commented 3 years ago

@galenlynch Now that you've highlighted a lot of these issues (thanks!) I'm keen to get them squashed. Are there any you're actively working on, and others I should start to look into?

galenlynch commented 3 years ago

Yeah I've fixed all of them in a branch on my fork... however I might have been a bit over zealous with some changes. While I've tried to keep the interface as consistent as possible, I have changed the fields of some types, and the encoding workflow is a bit changed. I've been debating about what I should do with it, and also don't know where to find 10 bit-depth videos to build tests around.

I can put some effort into tidying it up into a PR, if you're ok with some breaking changes. FWIW, I have been using it for my own stuff quite happily for the past two weeks or so.

galenlynch commented 3 years ago

Here is the diff of the branch I've been working on against VIdeoIO master:

https://github.com/JuliaIO/VideoIO.jl/compare/master...galenlynch:add_10bit_gray_encoding

IanButterworth commented 3 years ago

Sounds good. I'm all in favor of changes that make things better, changing or not. Obviously we'd release it with the appropriate version bump

I've been thinking that the encoding workflow could be better, so keen to see what you've come up up with

IanButterworth commented 3 years ago

Somehow missed your second message yesterday. Please feel free to open a PR / draft-PR. I'd be happy to review & try it out, even if it's early

IanButterworth commented 3 years ago

@galenlynch just a friendly reminder that I'm happy to review & try your branch out, just waiting for a PR

galenlynch commented 3 years ago

I'm working on the PR text now, but it will take a bit since there's quite a few changes. I'll make a draft PR once I'm finished with the PR description, though it will be missing tests.

galenlynch commented 3 years ago

@ianshmean I made a draft PR. Typing it took a while. I might have glossed over some details. As you can see, it's a massive unwieldy PR. Apologies in advance!

I do think it fixes many bugs, leans on libav's utilities more heavily, simplifies some of VideoIO's code, and adds new features. So even though it's pretty massive, I hope that it's worth the look.

galenlynch commented 3 years ago

I no longer think this is a problem. As long as users are able to change the gop setting, then using an encoder's default seems fine.