search

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

Undocumented change to `read!` between VideoIO versions 0.7.1 and 0.7.2 causes errors #253

Closed galenlynch closed 4 years ago

galenlynch commented 4 years ago

I have been using 0.7.1 for a while, but noticed that my client code is broken on master. After a bit of git-blaming, I found that this line in commit eaf4dbc (part of PR #245) is causing breakage for me, although I may have been abusing the API and it's just now come back to haunt me.

Specifically, I mostly work with images and frames of movies in a "scan-line major", or image-x major, memory format as many libraries expect that, including FFMPEG itself. However, VideoIO instead mostly assumes a row major format, where rows of a matrix are equated with image y dimension. Until eaf4dc, I simply used the parent array underlying the PermutedDimsArray view returned by VideoIO.read(r::VideoReader), and passed that parent array to subsequent calls to VdeoIO.read!(r::VideoReader, arr::VidArray) to read a video.

While the documentation says nothing about what type of array VideoIO.read! expects, the fact that VidArray{T,N} = Union{Array{T,N},PermutedArray{T,N}} led me to believe that passing it regular arrays was fine as far as the supported API was concerned, and obviously read! did not care since no permutation was being done, and in the end all the read! function was doing with buf was passing a pointer to the image-x major buffer to FFMPEG.

eaf4dc directly accesses the parent field of the buf::VidArray parameter, which is fine if buf is a PermutedArray but causes an error if it is the other portion of the VidArray union, namely a normal array. This line could be replaced by parent(buf), which would work in either case, or could also just be eliminated since pointer(buf) === pointer(parent(buf)), and nothing else is done with the parent array. I'll make a PR that does the latter soon, but that does not address the potential breakage that users will experience if they check out VideoIO 0.7.2. I'm unsure how to fix that breakage, so I'm positing this issue independent of my upcoming PR that will fix the underlying bug. Also some clarification of what is considered supported behavior would probably help: is read! with normal arrays supported?

IanButterworth commented 4 years ago

Ah.. Indeed, that was an unintended breakage. For your PR, adding unit test would be appreciated

galenlynch commented 4 years ago

I think the tests that I added cover both cases.

On Mon, Aug 3, 2020, 10:47 PM Ian Butterworth notifications@github.com wrote:

Ah.. Indeed, that was an unintended breakage. For your PR, adding unit test would be appreciated

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JuliaIO/VideoIO.jl/issues/253#issuecomment-668347603, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUDZT7EVEFDBUWUZBJJOUDR65YTXANCNFSM4PTWEEDA .