fix error of `opencamera()`

[hhaensel]

This PR addresses #341. I compared both solutions proposed in the chat and found that cconvert() is preferred over unsafe_convert() as I assumed, so I implemented the solution for cconvert().

I was not completely sure whether convert() is needed somewhere in the code - although I couldn't find any evidence in the code - so I kept it.

EDIT: added "not" in the phrase above...

[codecov[bot]]

Codecov Report

Merging #345 (fadb4bc) into master (7149e47) will increase coverage by 0.16%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #345      +/-   ##
==========================================
+ Coverage   82.00%   82.16%   +0.16%     
==========================================
  Files          10       10              
  Lines        1239     1245       +6     
==========================================
+ Hits         1016     1023       +7     
+ Misses        223      222       -1     

Impacted Files	Coverage Δ
src/avdictionary.jl	8.82% <0.00%> (ø)
src/avio.jl	82.51% <0.00%> (+0.24%)	:arrow_up:
src/avframe_transfer.jl	81.69% <0.00%> (+0.46%)	: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 7149e47...fadb4bc. Read the comment docs.

[louie-github]

Just a little comment: do you need to import cconvert? You're referencing it from Base via the dot operator.

[hhaensel]

Very true, importing is not needed. I shall change that.

[louie-github]

Hm. I just read through the Julia docs and found this:

cconvert(T,x)

Convert x to a value to be passed to C code as type T, typically by calling convert(T, x).

In cases where x cannot be safely converted to T, unlike convert, cconvert may return an object of a type different
from T, which however is suitable for unsafe_convert to handle. The result of this function should be kept valid
(for the GC) until the result of unsafe_convert is not needed anymore. This can be used to allocate memory that will
be accessed by the ccall. If multiple objects need to be allocated, a tuple of the objects can be used as return
value.

Neither convert nor cconvert should take a Julia object and turn it into a Ptr.

What struck out to me "Neither convert nor cconvert should take a Julia object and turn it into a Ptr." So maybe a different solution should be proposed so we don't convert it into a Ptr?

You can see it in julia/essentials.jl, as well as check what cconvert actually does.

I'm not really sure how I'd personally solve this problem.

[hhaensel]

Hm, although I went through the docs that you mentioned before I submitted the PR it seems that I hadn't completely understood how cconvert and unsafe_convert should be used. Re-reading the docs it seems to me that the correct way would be to use unsafe_convert rather than cconvert. Furthermore, when I look through the code, I find many definitions of unsafe_convert and none with cconvert, except the import in VideoIO. @louie-github would you agree that we rather delete the convert and cconvert methods in avdictionary.jl and add an unsafe_convert instead?

[hhaensel]

Sorry, some more thoughts. (I'm not the C-API guy).

The cconvert implementation is probably correct, as doesn't return a Ptr but the Ref of a ptr. An unsafe_convert will probably follow and do the Ptr conversion.

Anyone here, who knows more details? Always happy to learn new things ...

[yakir12]

This might be unrelated, but I tested this on a Raspberry Pi and it still errored with:

(tmp) pkg> st
      Status `~/tmp/Project.toml`
  [d6d074c3] VideoIO v0.9.6 `https://github.com/hhaensel/VideoIO.jl#hh-patch-opencamera`

julia> using VideoIO

julia> cam = VideoIO.opencamera()
ERROR: video stream 1 not found
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] VideoIO.VideoReader(avin::VideoIO.AVInput{String}, video_stream::Int32; transcode::Bool, target_format::Nothing, pix_fmt_loss_flags::Int32, target_colorspace_details::Nothing, allow_vio_gray_transform::Bool, swscale_options::NamedTuple{(), Tuple{}}, sws_color_options::NamedTuple{(), Tuple{}}, thread_count::Int32)
   @ VideoIO ~/.julia/packages/VideoIO/8OFog/src/avio.jl:257
 [3] VideoIO.VideoReader(avin::VideoIO.AVInput{String}, video_stream::Int32) (repeats 2 times)
   @ VideoIO ~/.julia/packages/VideoIO/8OFog/src/avio.jl:253
 [4] #opencamera#26
   @ ~/.julia/packages/VideoIO/8OFog/src/avio.jl:1017 [inlined]
 [5] opencamera(::String, ::Ptr{VideoIO.libffmpeg.AVInputFormat}, ::VideoIO.AVDict) (repeats 2 times)
   @ VideoIO ~/.julia/packages/VideoIO/8OFog/src/avio.jl:1016
 [6] top-level scope
   @ REPL[6]:1

See issue #346

[IanButterworth]

@melonedo @Gnimuc perhaps you could advise the right strategy here?

When merging & releasing https://github.com/JuliaIO/VideoIO.jl/pull/333 I forgot that VideoIO.opencamera() doesn't have test coverage, and evidently broke.

[melonedo]

IIRC, the rules for cconvert and unsafe_convert is that cconvert allocates memory, while unsafe_convert GC.@preserves memory. Since AVDict holds the underlying AVDictionary, you would want to use unsafe_convert in this case to prevent it from accidentally being freed before the C function is called.

[IanButterworth]

Ok, thanks.

I tried defining Base.unsafe_convert(::Type{Ptr{Ptr{AVDictionary}}}, d::AVDict) = d.ref_ptr_dict but I get

julia> VideoIO.opencamera()
ERROR: TypeError: in ccall argument 4, expected Ptr{Ptr{VideoIO.libffmpeg.AVDictionary}}, got a value of type Base.RefValue{Ptr{VideoIO.libffmpeg.AVDictionary}}
Stacktrace:
 [1] avformat_open_input(ps::VideoIO.NestedCStruct{VideoIO.libffmpeg.AVFormatContext}, url::String, fmt::Ptr{VideoIO.libffmpeg.AVInputFormat}, options::VideoIO.AVDict)
   @ VideoIO.libffmpeg ~/Documents/GitHub/VideoIO.jl/lib/libffmpeg.jl:8571

From the Ref docs, I expected the conversion of RefValue to Ptr to happen automatically

When passed as a `ccall` argument (either as a `Ptr` or `Ref` type), a `Ref`
object will be converted to a native pointer to the data it references.
For most `T`, or when converted to a `Ptr{Cvoid}`, this is a pointer to the
object data. When `T` is an `isbits` type, this value may be safely mutated,
otherwise mutation is strictly undefined behavior.

https://docs.julialang.org/en/v1/base/c/#Core.Ref

So it's not clear to me what the fix is

[Gnimuc]

To clarify a bit, you could use the following mental model when working with cconvert and unsafe_convert.

# `ccall(..., ..., (..., T), ..., x)` is basically equivalent to:
x_cc = Base.cconvert(T, x)
x_uc = Base.unsafe_convert(T, x_cc)
GC.@preserve x_cc begin
     # doing the actual call with `x_uc` 
end

In the case of AVDict, you can overload either cconvert or unsafe_convert.

mutable struct AVDict <: AbstractDict{String, String}
    ref_ptr_dict::Ref{Ptr{AVDictionary}}
end

But if AVDict is defined in the following way, you can only overload unsafe_convert.

mutable struct AVDict <: AbstractDict{String, String}
    ref_ptr_dict::Ptr{Ptr{AVDictionary}}
end

[Gnimuc]

From the Ref docs, I expected the conversion of RefValue to Ptr to happen automatically

As explained above, ccall only does cconvert and unsafe_convert, there is no such auto-conversion. (EDIT: the auto-conversion you see in other cases is implemented in Julia Base through this cconvert+unsafe_convert mechanism.)

ERROR: TypeError: in ccall argument 4, expected Ptr{Ptr{VideoIO.libffmpeg.AVDictionary}}, got a value of type Base.RefValue{Ptr{VideoIO.libffmpeg.AVDictionary}}

ccall expects a Ptr{Ptr{AVDictionary}} but the output of Base.unsafe_convert is of type Ref, that's what the error yields. The correct way to overload unsafe_convert is to use the method proposed in #341.

[IanButterworth]

@gnimuc In its current form, this PR doesn't work for me (MacOS, julia 1.6)

julia> VideoIO.opencamera()
ERROR: MethodError: no method matching unsafe_convert(::Type{Ptr{Ptr{VideoIO.libffmpeg.AVDictionary}}}, ::VideoIO.AVDict)
Closest candidates are:
  unsafe_convert(::Type{Ptr{T}}, ::LinearAlgebra.Transpose{var"#s814", var"#s813"} where {var"#s814", var"#s813"<:(AbstractVecOrMat{T} where T)}) where T at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/adjtrans.jl:222
  unsafe_convert(::Type{Ptr{T}}, ::SubArray{T, N, P, var"#s77", L} where {var"#s77"<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}}, N} where N}, L}) where {T, N, P} at subarray.jl:426
  unsafe_convert(::Type{Ptr{T}}, ::SubArray{T, N, P, var"#s77", L} where {var"#s77"<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}, Base.ReshapedArray{T, N, A, Tuple{}} where {T, N, A<:AbstractUnitRange}}, N} where N}, L}) where {T, N, P} at reshapedarray.jl:292
  ...
Stacktrace:
 [1] avformat_open_input(ps::VideoIO.NestedCStruct{VideoIO.libffmpeg.AVFormatContext}, url::String, fmt::Ptr{VideoIO.libffmpeg.AVInputFormat}, options::VideoIO.AVDict)
   @ VideoIO.libffmpeg ~/Documents/GitHub/VideoIO.jl/lib/libffmpeg.jl:8571
 [2] open_avinput(avin::VideoIO.AVInput{String}, source::String, input_format::Ptr{VideoIO.libffmpeg.AVInputFormat}, options::VideoIO.AVDict)
   @ VideoIO ~/Documents/GitHub/VideoIO.jl/src/avio.jl:195
 [3] VideoIO.AVInput(source::String, input_format::Ptr{VideoIO.libffmpeg.AVInputFormat}, options::VideoIO.AVDict; avio_ctx_buffer_size::Int64)
   @ VideoIO ~/Documents/GitHub/VideoIO.jl/src/avio.jl:218
 [4] VideoIO.AVInput(source::String, input_format::Ptr{VideoIO.libffmpeg.AVInputFormat}, options::VideoIO.AVDict)
   @ VideoIO ~/Documents/GitHub/VideoIO.jl/src/avio.jl:207
 [5] opencamera(::String, ::Ptr{VideoIO.libffmpeg.AVInputFormat}, ::VideoIO.AVDict; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ VideoIO ~/Documents/GitHub/VideoIO.jl/src/avio.jl:1024
 [6] opencamera(::String, ::Ptr{VideoIO.libffmpeg.AVInputFormat}, ::VideoIO.AVDict) (repeats 2 times)
   @ VideoIO ~/Documents/GitHub/VideoIO.jl/src/avio.jl:1024
 [7] top-level scope
   @ REPL[3]:1

[IanButterworth]

Thanks all. Sorry for the delay

[melonedo]

In the case of AVDict, you can overload either cconvert or unsafe_convert.

I doubt whether it is the case here. If we define cconvert to return a pointer or a reference to the pointer, and the garbage collector is run just before the ccall expression, which may call the finalizer of AVDict that frees the dictionary, creating a dangling pointer. So we can only overload unsafe_convert to return the pointer while keeping cconvert a no-op (Base.cconvert(_, x::AVDict)=x, this is probably the default) to make sure the AVDict object itself is alive throughout the c function.

[Gnimuc]

If we define cconvert to return a pointer

It's wrong to let cconvert return a Ptr because a Ptr can not be GC.@preserved(a Ptr doesn't have a solid (heap) address, it could be located in certain register or even optimized out (GC rooting runs after LLVM optimization passes.)).

As mentioned in https://github.com/JuliaIO/VideoIO.jl/pull/345#issuecomment-1060182721, the simple mental model is that the Julia object returned by cconvert will be automatically GC.@preserved. Note that, this object should be able to be preserved.

or a reference to the pointer,

Returning a Ref is the right way as this type can be GC.@preserved.

and the garbage collector is run just before the ccall expression, which may call the finalizer of AVDict that frees the dictionary,

The finalizer will not be triggered because AVDict's field is still being referenced during the invoking of ccall.

[melonedo]

Ptr can not be GC.@preserved

I assume this is because you want to preserve the dict object through a reference to its field, since it is totally valid for Base.cconvert(Int, 1) to return 1.

The finalizer will not be triggered because AVDict's field is still being referenced during the invoking of ccall.

What counts as a reference to an object is beyond my knowledge, can you recommend some materials on this?

[Gnimuc]

It's wrong to let cconvert return a Ptr

since it is totally valid for Base.cconvert(Int, 1) to return 1.

1(of type Int) is not a Ptr.

What counts as a reference to an object is beyond my knowledge, can you recommend some materials on this?

In the example below, the field x of Foo is marked as AddressSpace (10) which means it's a Julia object allocated and tracked by Julia's GC. The struct Foo is marked as AddressSpace (11) which means it's Derived from objects managed by GC and hence should also be treated carefully when GC runs. The "reference" I wrote above means that it's being GC-preserved/tracked(In fact, GC.@preserved is not used in the actual implementation, there is a low-level function call that has extra arguments dedicated for gc roots). IIRC, this is mentioned in certain discourse posts/docs on how GC is implemented.

julia> struct Foo
         x::Ref{Int}
         y::Ref{Float32}
         z::Int
       end

julia> x = Foo(1,2,3)
Foo(Base.RefValue{Int64}(1), Base.RefValue{Float32}(2.0f0), 3)

julia> f(obj) = obj.x 
f (generic function with 1 method)

julia> @code_llvm raw=true f(x)
;  @ REPL[3]:1 within `f`
define nonnull {} addrspace(10)* @julia_f_197({ {} addrspace(10)*, {} addrspace(10)*, i64 } addrspace(11)* nocapture nonnull readonly align 8 dereferenceable(24) %0) #0 !dbg !5 {
top:
; ┌ @ Base.jl:42 within `getproperty`
   %1 = getelementptr inbounds { {} addrspace(10)*, {} addrspace(10)*, i64 }, { {} addrspace(10)*, {} addrspace(10)*, i64 } addrspace(11)* %0, i64 0, i32 0, !dbg !7
   %2 = load atomic {} addrspace(10)*, {} addrspace(10)* addrspace(11)* %1 unordered, align 8, !dbg !7, !tbaa !11, !nonnull !4
; └
  ret {} addrspace(10)* %2, !dbg !10