After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 332031 - [PATCH] avimux ported to 0.10
[PATCH] avimux ported to 0.10
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.2
Other Linux
: Normal normal
: 0.10.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2006-02-21 13:27 UTC by Mark Nauwelaerts
Modified: 2006-04-27 20:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
possible port to 0.10 (8.65 KB, patch)
2006-02-21 13:32 UTC, Mark Nauwelaerts
none Details | Review
possible port to 0.10 (34.67 KB, patch)
2006-02-21 15:10 UTC, Mark Nauwelaerts
reviewed Details | Review
Small patch to prevent possible SEGV-fault (1.61 KB, patch)
2006-04-18 19:38 UTC, Mark Nauwelaerts
reviewed Details | Review
patch (includes unit test) (9.93 KB, patch)
2006-04-20 21:17 UTC, Mark Nauwelaerts
committed Details | Review

Description Mark Nauwelaerts 2006-02-21 13:27:37 UTC
(summary says it all :-) )
avidemux has been ported to 0.10, but avimux not
(and has been accordingly disabled in Makefile.am etc)
Comment 1 Mark Nauwelaerts 2006-02-21 13:32:23 UTC
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.
Comment 2 Edward Hervey 2006-02-21 14:44:09 UTC
what kind of format is that patch in ??? it's not usable.
Comment 3 Mark Nauwelaerts 2006-02-21 15:10:23 UTC
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
Comment 4 Luca Ognibene 2006-02-21 15:14:31 UTC
isn't always a good idea to start porting from 0.8 CVS ?
Comment 5 Mark Nauwelaerts 2006-02-28 22:31:32 UTC
(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).
Comment 6 Mark Nauwelaerts 2006-04-18 19:38:37 UTC
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
Comment 7 Tim-Philipp Müller 2006-04-18 20:01:27 UTC
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 :)

Comment 8 Mark Nauwelaerts 2006-04-20 21:17:07 UTC
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.
Comment 9 Tim-Philipp Müller 2006-04-27 09:26:54 UTC
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.
Comment 10 Tim-Philipp Müller 2006-04-27 15:00:08 UTC
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

Comment 11 Mark Nauwelaerts 2006-04-27 20:32:13 UTC
(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.