Remove old encode functions

[IanButterworth]

I think it may just be best if we remove the old functions that were superseded in https://github.com/JuliaIO/VideoIO.jl/pull/279 given that they fix a few issues, and are more streamlined.

I've only switched out encodevideo -> encode_mux_video so far.

One issue though, I tried switching out to the new encoder_settings dict from [:color_range=>2, :priv_data => ("crf"=>"0","preset"=>"medium")] to the new named tuples format of (color_range=2, priv_data = (crf="0", preset="medium")) but that throws

Got exception outside of a @test
  Could not set class option priv_data to (crf = "0", preset = "medium"): got error -1414549496

It seems that when the color_range is specified, the priv_data part needs to be specified through the encoder_private_settings kwarg, like this

encoder_settings = (color_range=2,)
encoder_private_settings = (crf="0", preset="medium")
VideoIO.encode_mux_video(vidpath, imgstack, encoder_settings = encoder_settings,
                            encoder_private_settings = encoder_private_settings)

It seems to make some sense, but might make it less clear for people to switch over

What do you think @galenlynch ?

[codecov[bot]]

Codecov Report

Merging #301 (6120cd3) into master (fbc7e73) will increase coverage by 2.52%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage   79.83%   82.35%   +2.52%     
==========================================
  Files          17       17              
  Lines        1344     1196     -148     
==========================================
- Hits         1073      985      -88     
+ Misses        271      211      -60     

Impacted Files	Coverage Δ
src/VideoIO.jl	53.33% <ø> (ø)
src/encoding.jl	93.20% <66.66%> (+7.93%)	:arrow_up:
src/ffmpeg/AVUtil/src/AVUtil.jl	66.66% <0.00%> (-33.34%)	:arrow_down:
src/testvideos.jl	80.00% <0.00%> (-7.50%)	:arrow_down:
src/avio.jl	81.25% <0.00%> (+0.25%)	:arrow_up:
src/avframe_transfer.jl	83.41% <0.00%> (+12.80%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fbc7e73...6120cd3. Read the comment docs.

[galenlynch]

Thanks for doing this. I'm also happy to help out with deprecations and removal of the old workflow.

In #279 I tried to simplify the code in VideoIO that handles encoder settings by using libav's functions for manipulating AVOptions directly. This means the option names should be the same as those used in ffmpeg's CLI. You're right that this will be a bit of a change from the previous interface, which scanned through the options to locate priv_data options and handle them separately. However, because there is no priv_data option in ffmpeg's CLI, passing it will cause the error with the new interface, like you show above. I think in the long run we should eliminate the priv_data nested option that is specific to VideoIO, as it's not what people familiar with ffmpeg would expect, requires more code in VideoIO, and also just seems more complicated to use.

If users don't know or care which of their options are generic to all encoders, or are instead private options for a particular encoder, then they don't actually need to use the encoder_private_settings keyword. For example, in your example above, you could actually just do:

encoder_settings = (color_range=2, crf=0, preset="medium")
# using Julia's new keyword syntax, equivalent to encoder_settings = encoder_settings
VideoIO.encode_mux_video(vidpath, imgstack; encoder_settings) 

This works because the VideIO's set_class_option defaults to using the search strategy of AV_OPT_SEARCH_CHILDREN, so unrecognized options passed to the AVCodecContext will be matched against the AVOptions in priv_data.

In terms of providing support for users switching over, maybe we could use the old behavior but throw a deprecation warning if priv_data is encountered. However, I think this behavior should be deprecated.

[IanButterworth]

Nice. The AV_OPT_SEARCH_CHILDREN setup makes this simpler. I've added a check and error if priv_data is provided.

btw, thanks for the offer of help.

I'm trying to keep momentum up on this, aiming to be ready for a v0.9 release by the end of this week, so I'd appreciate if you could review this, and if all good I can merge within a day or so.

Once this is merged:

• Check and update docs (I'd appreciate if you could take a look at the docs with fresher eyes than mine.. we should also set up proper doctests)
• Update Readme (Can be simpler)
• Setup API functions for FileIO interface
• Create a NEWS.md to give examples of how to update to 0.9 syntax

Also, I think we should keep an eye on coverage, and see if there are any easy wins in getting it up in a meaningful way.

[galenlynch]

Is the goal of this particular PR to mark the old functionality as deprecated but allow it to still work, or instead to remove the old functionality that has been superseded?

[IanButterworth]

Given it was broken, I think it's better to just remove and make a very visible changelog and upgrade guide. So this removes the old functions and switches tests over to the new functions.

[galenlynch]

Should we rename encoder_settings to encoder_options, which would be more similar to ffmpeg?

[galenlynch]

I see. I can submit a PR against your branch that removes some more code that's only necessary for the old workflow, if that's alright with you.

[IanButterworth]

That'd be great. Thanks. And feel free to rename where appropriate.

I've also thought maybe we want to rename encode_mux_vídeo as a non-exported VideoIO.save

[IanButterworth]

Let's merge this, then do a rename PR to master