Created attachment 59848 [details] [review]
possible port to 0.10

A bit of a big(ger) diff that port CVS release 0.10.2 avimux (actually not 0.10) to 0.10.  Note that changes to Makefile.am and gstavi.c for plugin activation are also included in the patch.

what kind of format is that patch in ??? it's not usable.

Created attachment 59854 [details] [review]
possible port to 0.10

hm, the previous attachment/patch was the gzipped-version of this one.
Now attached in really "plain text" in case the other one got mangled too much

isn't always a good idea to start porting from 0.8 CVS ?

(In reply to comment #4)
> isn't always a good idea to start porting from 0.8 CVS ?
> 

I would say, yes.  In this case, however, if I read CVS right, the 0.8 branch has not seen activity in about 10 months on this part, and the avi(de)mux activity in 0.10 is (much) more recent.  I assume this corresponds with 0.10 now being the new "stable line" of work/maintenance.  In any event, the diff aims toward HEAD (well, close to HEAD, 0.10.2).

Created attachment 63818 [details] [review]
Small patch to prevent possible SEGV-fault

Through further use/testing, I noticed a few minor things in the original patch that cause SEGV if only 1 pad of avimux is connected.  A small patch is attached (it is additive, so it patches the result of applying the previous patch).

I'm a bit curious about this patch's status; is it planned to be committed ?
I was hoping/planning to add some other enhancement to avimux (e.g. ODML index support), but as it is currently a bit in "mid-air" ...

Thanks

The patch needs reviewing, which is a bit of a tedious task, certainly will take more than just a few minutes :)

Plugins should in general be ported based on the 0.8 version, because the 0.10 branch was branched off ~ January 2005 and we don't want to lose all the fixes between then and when people stopped working on 0.8. However, I've checked the CVS history in the 0.8 branch and there weren't any noteworthy fixes there, so that's not an issue, which is good news.

What would be very helpful and confidence-inspiring (I think) are some simple test cases for the check framework (see tests/check/*), ie. trying to mux some raw audio/video using videotestsrc/audiotestsrc or so :)

Created attachment 63994 [details] [review]
patch (includes unit test)

Ah, it seems that with the rules about "moving" plugins into '-good', one can't just sneak something in there :)  (nor should one)

As suggested, a (very simple) unit test for avimux is attached.
In the course of its creation, 2 small mem-leaks were found :( and plugged :)

Note that this patch is (again) additive.

Hi Mark, great work!

Made the patch apply against CVS HEAD and compile with -Wall -Werror and did some clean-ups here and there (most of which I forgot or which were minor things).

Here are some small things I ran into (fixed in my local copy):

- GST_PAD_PARENT does not return a reference, no gst_object_unref'ing
  needed, whereas gst_pad_get_parent() does return a reference and one
  needs to gst_object_unref() when done (these might not have been part
  of your patch though, but part of a mass update in CVS, didn't bother
  to check)

- gst_pad_get_name() returns a newly alloc'ed string which must be freed
  by g_free(), so it shouldn't be used in debug statements. GST_PAD_NAME
  or GST_DEBUG_PAD_NAME (with %s:%s string formatter) are better for
  debug statements (this is different from 0.8 where these didn't leak).

- one should generally set correct caps on all outgoing buffers
  (but absolutely must do that for the first outgoing buffer).

- gst_avimux_stop_file():
  GST_ELEMENT_ERROR should be used only for fatal things. If it
  is fatal, then you should also return GST_FLOW_ERROR, otherwise
  GST_ELEMENT_WARNING should do the job (otherwise the application
  won't know that the error message you send is not fatal).

- gst_avimux_handle_event():
  don't leak taglist extracted from tag event.

- commented out audio_pad_eos and video_pad_eos members - are they
  actually needed/used anywhere?

- gst_avimux_strip_buffer():
  can't assume it's safe to modify metadata on buffers like this (even
  though it probably is, but still). Must use
  gst_buffer_make_metadata_writable().

- the assumption is that the incoming streams are perfect streams, right?
  (ie. no gaps)

- gst_avimux_do_one_buffer():
  are avimux->video_buffer_queue and avimux->audio_buffer_queue needed
  for anything? Looks to me like they could just as well be replaced
  by local variables.

- gst_avimux_do_one_buffer():
  flow return on EOS should be GST_FLOW_UNEXPECTED not GST_FLOW_WRONG_STATE.

- gst_avimux_write_index():
  needs to return GST_FLOW_OK at the end

Still tracking down something related to the test case, will post updated patch as soon as that's fixed.

Tested and committed to CVS - Worked out of the box for me, nice job!

2006-04-27  Tim-Philipp Müller  <tim at centricular dot net>

        Patch by: Mark Nauwelaerts  <manauw at skynet dot be>

        * gst/avi/Makefile.am:
        * gst/avi/gstavi.c: (plugin_init):
        * gst/avi/gstavimux.c: (gst_avi_mux_get_type),
        (gst_avi_mux_base_init), (gst_avi_mux_finalize),
        (gst_avi_mux_class_init), (gst_avi_mux_init),
        (gst_avi_mux_vidsink_set_caps), (gst_avi_mux_audsink_set_caps),
        (gst_avi_mux_pad_link), (gst_avi_mux_pad_unlink),
        (gst_avi_mux_request_new_pad), (gst_avi_mux_release_pad),
        (gst_avi_mux_write_tag), (gst_avi_mux_riff_get_avi_header),
        (gst_avi_mux_riff_get_avix_header),
        (gst_avi_mux_riff_get_video_header),
        (gst_avi_mux_riff_get_audio_header), (gst_avi_mux_add_index),
        (gst_avi_mux_write_index), (gst_avi_mux_bigfile),
        (gst_avi_mux_start_file), (gst_avi_mux_stop_file),
        (gst_avi_mux_restart_file), (gst_avi_mux_handle_event),
        (gst_avi_mux_fill_queue), (gst_avi_mux_send_pad_data),
        (gst_avi_mux_strip_buffer), (gst_avi_mux_do_audio_buffer),
        (gst_avi_mux_do_video_buffer), (gst_avi_mux_do_one_buffer),
        (gst_avi_mux_loop), (gst_avi_mux_collect_pads),
        (gst_avi_mux_get_property), (gst_avi_mux_set_property),
        (gst_avi_mux_change_state):
        * gst/avi/gstavimux.h:
          Port AVI muxer to GStreamer-0.10 (#332031).

        * tests/check/Makefile.am:
        * tests/check/elements/avimux.c:
        * tests/check/elements/.cvsignore:
          Add unit test for AVI muxer.

My above statement about gst_avimux_handle_event() leaking the list was rubbish, we don't get a copy of the list, so your original code there was correct.

However, I found I had to add a

    avimux->audiocollectdata = NULL;
and
    avimux->videocollectdata = NULL;

in gst_avi_mux_pad_unlink() though to avoid an invalid memory access when using both a video stream and an audio stream. This doesn't happen with the unit test though.


Test pipeline:

 $ OIL_CPU_FLAGS=0 G_SLICE=always-malloc valgrind --leak-check=yes gst-launch-0.10 videotestsrc num-buffers=500 ! video/x-raw-yuv,format=\(fourcc\)I420,width=320,height=240 ! xvidenc !  queue ! mux.  audiotestsrc num-buffers=500 ! audioconvert ! audio/x-raw-int,rate=44100,channels=2 ! lame ! queue ! mux.  avimux name=mux ! filesink location=test.avi

(In reply to comment #9)
> Hi Mark, great work!
OK, thanks for the feedback/comments :), some nice guidelines to bear in mind.

> - commented out audio_pad_eos and video_pad_eos members - are they
>   actually needed/used anywhere?
It seems they once were in the old muxing code, but no longer now that GstCollectPads is in the game; overlooked this to clean up.

> - the assumption is that the incoming streams are perfect streams, right?
>   (ie. no gaps)
I hope not :)
That is, the mux-decision is "segment-aware", and decides what to mux in next based on the running-time (computed on buffer-time and segment info).
After all, a muxer should "create" (in a sense) a "new" media-timeline.
So, if the gaps/discontinuities are (properly) signaled using segments, it should be OK (if not, there could indeed be an imbalance between pads, and therefore problems).  In particular, avimux can handle a gap/discontinuity that results from sending a seek to the pipeline.  At least, I have used this many times to do some simple cut-list transcoding using (segment-)seek.

> - gst_avimux_do_one_buffer():
>   are avimux->video_buffer_queue and avimux->audio_buffer_queue needed
>   for anything? Looks to me like they could just as well be replaced
>   by local variables.
Indeed, I think I almost intended to do so, but then apparently decided to stick a bit closer to the original code.