search

JuliaIO / VideoIO.jl

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

Fix typedef, tidy up tests #300

Closed IanButterworth closed 3 years ago

IanButterworth commented 3 years ago

Updates to CI setup, removing redundant files, reorganizing tests because we had a lot going on in a single file

IanButterworth commented 3 years ago

@galenlynch It seems something about ReinterpretArray has changed in 1.6

MethodError: no method matching transfer_img_bytes_to_frame_plane!(::Ptr{UInt8}, ::ReinterpretArray{UInt8, 2, N6f10, ReinterpretArray{N6f10, 2, Gray{N6f10}, Matrix{Gray{N6f10}}, true}, false}, ::Int32, ::Int32, ::Int32, ::Int64)
  Closest candidates are:
    transfer_img_bytes_to_frame_plane!(::Any, ::StridedArray{T, N} where {T, N}, ::Any, ::Any, ::Any, ::Any) at /home/runner/work/VideoIO.jl/VideoIO.jl/src/avframe_transfer.jl:199

https://github.com/JuliaIO/VideoIO.jl/pull/300/checks?check_run_id=2104800042#step:4:445

In 1.5.4 we arrive at

julia> using ColorTypes, FixedPointNumbers

julia> Base.ReinterpretArray{UInt8,2,Gray{Normed{UInt8,8}},Array{Gray{Normed{UInt8,8}},2}} <: StridedArray
true

In 1.6.0-rc2

julia> using ColorTypes, FixedPointNumbers

julia> Base.ReinterpretArray{UInt8, 2, N0f8, Base.ReinterpretArray{N0f8, 2, Gray{N0f8}, Matrix{Gray{N0f8}}, true}, false} <: StridedArray
false

Maybe the problem should be caught further upstream because the outer function https://github.com/JuliaIO/VideoIO.jl/blob/0388303991844d602e90b22084ab0a969eb9b531/src/avframe_transfer.jl#L272-L273 has a looser definition for img than the function it passes to unchanged img to https://github.com/JuliaIO/VideoIO.jl/blob/0388303991844d602e90b22084ab0a969eb9b531/src/avframe_transfer.jl#L199-L201

IanButterworth commented 3 years ago

Relaxing to the same img::AbstractArray{UInt8} typedef for transfer_img_bytes_to_frame_plane! seems to fix it. I'm guessing the narrowing typedef here was just an unintentional inconsistency that slipped through given that for some reason it's ok on 1.5

IanButterworth commented 3 years ago

I'm going to merge because I want to move on to the next PR, but let me know if the typedef change wasn't appropriate @galenlynch

codecov[bot] commented 3 years ago

Codecov Report

Merging #300 (97a0668) into master (c128fca) will increase coverage by 3.74%. The diff coverage is 81.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
+ Coverage   76.09%   79.83%   +3.74%     
==========================================
  Files          14       17       +3     
  Lines         594     1344     +750     
==========================================
+ Hits          452     1073     +621     
- Misses        142      271     +129
Impacted Files Coverage Δ
src/VideoIO.jl 53.33% <ø> (-16.37%) :arrow_down:
src/avptr.jl 69.47% <69.47%> (ø)
src/avframe_transfer.jl 70.61% <70.61%> (ø)
src/avio.jl 81.00% <81.13%> (+7.40%) :arrow_up:
src/encoding.jl 85.26% <84.85%> (-2.80%) :arrow_down:
src/frame_graph.jl 99.18% <99.18%> (ø)
src/info.jl 100.00% <100.00%> (ø)
src/testvideos.jl 87.50% <100.00%> (-3.41%) :arrow_down:
src/util.jl 94.73% <100.00%> (+33.19%) :arrow_up:
src/ffmpeg/AVCodecs/src/AVCodecs.jl 66.66% <0.00%> (-33.34%) :arrow_down:
... and 10 more

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 0388303...97a0668. Read the comment docs.

galenlynch commented 3 years ago

I think relaxing the typebounds is fine, IRC the real safety check is the call to stride. I'll try to remember why I put that type bound there in the first place.

IanButterworth commented 3 years ago

Investigating an upstream fix https://github.com/JuliaLang/julia/pull/40027