fix error of `opencamera()` #345

hhaensel · 2022-01-20T23:10:48Z

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 · 2022-01-20T23:45:06Z

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%)`	⬆️
src/avframe_transfer.jl	`81.69% <0.00%> (+0.46%)`	⬆️

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 · 2022-01-24T18:39:54Z

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

hhaensel · 2022-01-24T19:36:58Z

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

louie-github · 2022-01-25T06:16:56Z

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 · 2022-01-25T08:49:45Z

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 · 2022-01-25T09:05:05Z

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 · 2022-02-17T11:55:48Z

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 · 2022-03-06T17:36:22Z

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

When merging & releasing #333 I forgot that VideoIO.opencamera() doesn't have test coverage, and evidently broke.

melonedo · 2022-03-07T00:10:39Z

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 · 2022-03-07T04:13:04Z

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 · 2022-03-07T04:29:37Z

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 · 2022-03-07T04:40:09Z

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.

Gnimuc · 2022-03-07T04:43:31Z

src/avdictionary.jl

@@ -29,6 +29,7 @@ end
 Base.empty!(d::AVDict) = libffmpeg.av_dict_free(d.ref_ptr_dict)

 Base.convert(::Type{Ref{Ptr{AVDictionary}}}, d::AVDict) = d.ref_ptr_dict


If this method is not used anywhere, it's better to remove it to avoid further confusion.

Good point. Without it tests pass, and VideoIO.opencamera() works locally.

src/avdictionary.jl

IanButterworth · 2022-03-07T17:19:52Z

Thanks all. Sorry for the delay

melonedo · 2022-03-08T14:26:57Z

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 · 2022-03-08T14:54:03Z

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 #345 (comment), 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 · 2022-03-08T15:21:23Z

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 · 2022-03-08T15:47:49Z

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

fix error of opencamera()

a500d54

remove import of cconvert

80dd4e4

Gnimuc approved these changes Mar 7, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

IanButterworth reviewed Mar 7, 2022

View reviewed changes

src/avdictionary.jl Outdated Show resolved Hide resolved

rm existing convert method

fadb4bc

IanButterworth merged commit 7d790a2 into JuliaIO:master Mar 7, 2022

This was referenced Mar 7, 2022

opencamera on a Raspberry Pi #346

Open

Can't get images from webcamera (Ubuntu Julia 1.7) #344

Closed

opencamera() not working and simultaneous encode/mux test reports as broken (Julia 1.6.3, VideoIO 0.9.6) #341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix error of `opencamera()` #345

fix error of `opencamera()` #345

hhaensel commented Jan 20, 2022 •

edited

Loading

codecov bot commented Jan 20, 2022 •

edited

Loading

louie-github commented Jan 24, 2022

hhaensel commented Jan 24, 2022

louie-github commented Jan 25, 2022

hhaensel commented Jan 25, 2022

hhaensel commented Jan 25, 2022

yakir12 commented Feb 17, 2022

IanButterworth commented Mar 6, 2022

melonedo commented Mar 7, 2022

IanButterworth commented Mar 7, 2022

Gnimuc commented Mar 7, 2022 •

edited

Loading

Gnimuc commented Mar 7, 2022 •

edited

Loading

Gnimuc Mar 7, 2022

IanButterworth Mar 7, 2022

This comment was marked as resolved.

IanButterworth commented Mar 7, 2022

melonedo commented Mar 8, 2022 •

edited

Loading

Gnimuc commented Mar 8, 2022

melonedo commented Mar 8, 2022

Gnimuc commented Mar 8, 2022 •

edited

Loading

		@@ -29,6 +29,7 @@ end
		Base.empty!(d::AVDict) = libffmpeg.av_dict_free(d.ref_ptr_dict)

		Base.convert(::Type{Ref{Ptr{AVDictionary}}}, d::AVDict) = d.ref_ptr_dict

fix error of opencamera() #345

fix error of opencamera() #345

Conversation

hhaensel commented Jan 20, 2022 • edited Loading

codecov bot commented Jan 20, 2022 • edited Loading

Codecov Report

louie-github commented Jan 24, 2022

hhaensel commented Jan 24, 2022

louie-github commented Jan 25, 2022

hhaensel commented Jan 25, 2022

hhaensel commented Jan 25, 2022

yakir12 commented Feb 17, 2022

IanButterworth commented Mar 6, 2022

melonedo commented Mar 7, 2022

IanButterworth commented Mar 7, 2022

Gnimuc commented Mar 7, 2022 • edited Loading

Gnimuc commented Mar 7, 2022 • edited Loading

Gnimuc Mar 7, 2022

Choose a reason for hiding this comment

IanButterworth Mar 7, 2022

Choose a reason for hiding this comment

This comment was marked as resolved.

IanButterworth commented Mar 7, 2022

melonedo commented Mar 8, 2022 • edited Loading

Gnimuc commented Mar 8, 2022

melonedo commented Mar 8, 2022

Gnimuc commented Mar 8, 2022 • edited Loading

fix error of `opencamera()` #345

fix error of `opencamera()` #345

hhaensel commented Jan 20, 2022 •

edited

Loading

codecov bot commented Jan 20, 2022 •

edited

Loading

Gnimuc commented Mar 7, 2022 •

edited

Loading

Gnimuc commented Mar 7, 2022 •

edited

Loading

melonedo commented Mar 8, 2022 •

edited

Loading

Gnimuc commented Mar 8, 2022 •

edited

Loading