Mean messes with video

[toots]

Need to test again, the code changed a lot.

[wiki]

 wrought iron stair railings  decorative railing  deck railings  railing

[toots]

I can confirm the bug. I'm highly suspicious that the call to content_of_type destroys the video layer and that we see in fact the memory, hence the messy output..

[toots]

More precisely:

              (* We are erasing the current layer. *)
              match acc with
                | (end_pos,content)::acc 
                  when content_has_type content content_type ->
                    (* We must re-use the previous layer. *)
                    frame.contents <- List.rev ((!!size, content)::acc) ;
                    content
                | _ ->
                    let content = create_content content_type in
                      frame.contents <- List.rev ((!!size, content)::acc) ;
                      content

In this case, we ask for mono and the content is stereo. Hence, I believe a new content is create. Unfortunately, in this case, the video content is also erased..

[toots]

Yep, I can confirm. If I manually save and restore the video data in mean.ml I get a correct video output.

However, I am not sure this is the correct solution, it is more likely that we should save the video layer when asking for a new content type that has (compatible) video, no ?

[mrpingouin]

Good catch, content_of_type is irrelevant here: mean modifies the existing content, it should use content. Using content_of_type is for operators that create new content, not for those who modify the existing content without changing its type.

[toots]

The following patch, which I believe is correct, trigers a segault:

--- conversions/mean.ml (révision 7317)
+++ conversions/mean.ml (copie de travail)
@@ -23,7 +23,6 @@
 open Source
 
 class mean ~kind (source:source) =
-  let ctype = Frame.type_of_kind kind in
 object (self)
   inherit operator kind [source] as super
 
@@ -34,12 +33,9 @@
 
   method private get_frame buf =
     let offset = AFrame.position buf in
-    let moffset = Frame.position buf in
     let buffer = source#get buf ; AFrame.content buf offset in
-    let dst = Frame.content_of_type buf moffset ctype in
-    let dst = dst.Frame.audio.(0) in
       for i = offset to AFrame.position buf - 1 do
-        dst.(i) <-
+        buffer.(0).(i) <-
           Array.fold_left (fun m b -> m +. b.(i)) 0. buffer
           /. float (Array.length buffer)
       done

Sefgault:

0x00000000006f0784 in ocaml_vorbis_encode_float (vdsp=22265016, vogg=22264280, data=22266648, _offs=<value optimized out>, _len=<value optimized out>) at vorbis_stubs.c:432
432           vorbis_buffer[c][i] = Double_field(datac, i + offs);
(gdb) bt
#0  0x00000000006f0784 in ocaml_vorbis_encode_float (vdsp=22265016, vogg=22264280, data=22266648, _offs=<value optimized out>, _len=<value optimized out>) at vorbis_stubs.c:432
#1  0x000000000071aedc in caml_c_call ()
#2  0x000000000153b728 in ?? ()
#3  0x00000000004d1d36 in camlOgg_muxer__encode_281 ()

To reprocuce:

output.file(%ogg(%theora,%vorbis(channels=1)),"/tmp/gni.ogv",mean(single("/tmp/bla.ogv")))

I do not know what to do here. More generaly, I am suspecting some kind of bug in the content management. There is another segfault with video (#354) which I have not been able to track down.

Basically, the above code should not, if in pure ocaml code, segfault. If the array is to small, usual ocaml code returns an exception.

However, we are using bindings which, purposedly, may access an array without these securities, in the C code of ocaml-vorbis, for instance, in order to optimize the code.

Hence, it could be the case that we do not manage properly the size of the arrays used for the contents..

[metamorph68]

(In [7339]) Fixed mean. The segault was in fact due to ocaml-vorbis assuming its encoded array has the exact number of channels defined for the encoder. This is fixed in [7338]

I have also added some more strict kind for the input source: the input source needs at least one audio channel now.

fixes #329

[metamorph68]

(In [7339]) Fixed mean. The segault was in fact due to ocaml-vorbis assuming its encoded array has the exact number of channels defined for the encoder. This is fixed in [7338]

I have also added some more strict kind for the input source: the input source needs at least one audio channel now.

fixes #329

[metamorph68]

(In [7339]) Fixed mean. The segault was in fact due to ocaml-vorbis assuming its encoded array has the exact number of channels defined for the encoder. This is fixed in [7338]

I have also added some more strict kind for the input source: the input source needs at least one audio channel now.

fixes #329