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

Allow read! to work with regular arrays, not just PermutedDimsArrays #254

Closed galenlynch closed 4 years ago

galenlynch commented 4 years ago

Commit eaf4dbc fixed a bug where FFMPEG's line size alignment requirements were not met, but in doing so unintentionally restricted the types of Julia arrays that would would work with VideoIO's read! from the original VidArray = Union{PermutedArray,Array} to just PermutedArrays. While the default return type of VideoIO's read is a PermutedDimsArray, this commit caused regular arrays to throw an error when used with read!, and in effect changed the (undocumented) API of read! between VideoIO versions v0.7.1 and v0.7.2. Furthermore, the fixed read! that respects FFMPEG's SIMD alignment requirements should work with either a normal Array, or instead a PermutedDimsArrays view of that array; the memory layout of both types of buffers are identical.

I have removed the line that caused the error when used with normal arrays: an attempt to access the parent field of the buffer. I have also changed the source of frame width and height information from the size of the permuted buffer to the width and height information given by the transcoder. This switch eliminates another bug when copying lines from the AVFrame managed by ffmpeg into the buffer used by Julia client code when that second buffer is a regular array. In addition to fixing this bug, the FFMPEG transcoder is a more authoritative source of information about frame dimensions, and indeed no checking was done to ensure that the height and width of the buffer matches the height and width of the transcoder.

I have also eliminated the temporary variables o_stride and o_size, which are never used.

I have added a bit of documentation to VideoIO's read! function, describing the valid types of buffers for read!.

Finally, I have added tests to ensure read! works with both PermutedDimsArrays as well as Arrays. In the process, I have refactored some code into helper functions that would otherwise be repeated for these tests.

Closes #253

codecov[bot] commented 4 years ago

Codecov Report

Merging #254 into master will increase coverage by 0.09%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
+ Coverage   77.00%   77.09%   +0.09%     
==========================================
  Files          14       14              
  Lines         600      598       -2     
==========================================
- Hits          462      461       -1     
+ Misses        138      137       -1     
Impacted Files Coverage Δ
src/avio.jl 79.15% <100.00%> (+0.18%) :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 1212211...1647085. Read the comment docs.

IanButterworth commented 4 years ago

Thanks for this. We should get a release out so I'll merge and tag