Closed IanButterworth closed 4 years ago
Merging #256 into master will decrease coverage by
0.25%
. The diff coverage is33.33%
.
@@ Coverage Diff @@
## master #256 +/- ##
==========================================
- Coverage 77.09% 76.83% -0.26%
==========================================
Files 14 14
Lines 598 600 +2
==========================================
Hits 461 461
- Misses 137 139 +2
Impacted Files | Coverage Δ | |
---|---|---|
src/encoding.jl | 89.62% <33.33%> (-1.35%) |
:arrow_down: |
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 d2166e5...07f756d. Read the comment docs.
Before #249, the frame rates passed to libavcodec
were constructed by direct calls to AVRational
, the two relevant calls are AVRational(1, framerate)
and AVRational(framerate, 1)
: no intermediate conversion to rational was done. Indeed, on v0.7.2, if I modify the frame rate in the "Encoding video across all supported colortypes" test block in /test/avio.jl
from 30 to 29.5 and try to run the encoding, here is the error that I get:
...
julia> encodedvideopath = VideoIO.encodevideo("testvideo.mp4",imgstack,framerate=29.5,AVCodecContextProperties=props, silent=true)
ERROR: InexactError: Int32(29.5)
Stacktrace:
[1] Int32 at ./float.jl:689 [inlined]
[2] convert at ./number.jl:7 [inlined]
[3] AVRational at /home/glynch/Documents/Code/VideoIO/src/ffmpeg/AVUtil/v56/libavutil_h.jl:640 [inlined]
[4] prepareencoder(::Array{UInt8,2}; framerate::Float64, AVCodecContextProperties::Array{Pair{Symbol,Tuple{Pair{String,String},Pair{String,String}}},1}, codec_name::String) at /home/glynch/Documents/Code/VideoIO/src/encoding.jl:120
[5] encodevideo(::String, ::Array{Array{UInt8,2},1}; AVCodecContextProperties::Array{Pair{Symbol,Tuple{Pair{String,String},Pair{String,String}}},1}, codec_name::String, framerate::Float64, silent::Bool) at /home/glynch/Documents/Code/VideoIO/src/encoding.jl:246
[6] top-level scope at REPL[23]:1
[7] eval(::Module, ::Any) at ./boot.jl:331
[8] eval_user_input(::Any, ::REPL.REPLBackend) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.4/REPL/src/REPL.jl:86
[9] run_backend(::REPL.REPLBackend) at /home/glynch/.julia/packages/Revise/BqeJF/src/Revise.jl:1184
[10] top-level scope at none:0
So the de facto API was that only integer frame rates were supported, and not floating point frame rates, even if the key word argument had no type restrictions (and defaulted to ::Any
). You're right that a floating point frame rate that was integer valued, e.g. 29.0, worked on v0.7.2 but I think that's effectively an integer. Therefore I don't agree that #249 caused a breaking change, it instead expanded the type bounds for framerate
that would work without error from any number that was integer valued (mostly <:Integer
), to Union{Integer, Rational}
.
It's true that the ffmpeg executable will accept floating point numbers and convert it internally to a rational, and that rational will accept a floating point and convert it to a rational. So I think it might make sense to expand the VideoIO API to include floating point framerate
s as well, however it won't work all of the time. Both the numerator and denominator have to be able to fit in Int32
fields as that is what is passed to libavcodec
. The real type bounds (types that won't error) on framerate
is certainly more restrictive than ::Any
, and I think Union{Rational, Integer}
is aligned with what will actually work, though as I just pointed out it won't work with all Rational
. Many floating point values will result in cryptic errors being thrown. As a user, I think it would be helpful if a more verbose error than an internal trunc
error was thrown.
Maybe you could wrap the call to AVRational
in a try catch block and throw an error message that explains what sort of values would work?
Also you're totally right that the framerate
keyword argument for mux
would work with a very broad range of things, so long as it was converted to a string format that the ffmpeg executable can parse. You're definitely right that the type narrowing for mux
was unintentional breakage.
Yeah. mux
was where I found the issue. It's a breaking change in the sense that it broke, but it shouldn't have been the way it was.. so that was more of an issue. Interpolating a kwarg that can be Any
into a cmd string isn't really good practice.
I'm keen to get a 0.7
release out that reverts the breaking change, but later we can become more specific.
I just pushed a change that errors if an invalid type is passed to mux
In #249 the framerate arg in
prepareencoder
andmux
was restricted toInteger
orRational
, however floats can also be passed as the conversion intoRational
will handle them.That PR also introduced a breaking change that I missed as the type of the kwargs became more limited, and it was actually possible to pass floats, as long as they rounded to
Integer
successfully.cc @galenlynch