Created attachment 60004 [details] [review]
Port of ffmuxers to 0.10

Created attachment 60410 [details] [review]
Port of ffmuxers to 0.10

This is updated patch against the latest CVS version

I am currently patching it up a little bit before commit. Is there any  major modifications in the new patch ?

No, this new version only fixes conflict in gstffmpeg.c and Makefile.am made by the inclusion of ffscaler patch.
Beside, as I have just observed, this patch includes gstffmpegprotocol.c two times in libgstffmpeg_la_SOURCES what is wrong.

Created attachment 60411 [details] [review]
Port of ffmuxers to 0.10

Accidentaly broken Makefile.am in the last patch fixed.

ok, I'm still going over some fixes. Once it's good I'll commit.

Several comments:
set/get_property are not needed, nor are the signal/property enums.

Around line 500 (/* set time */) you're setting pkt.pts twice, which means only the second one will be taken into account. Is that an error ?
    pkt.pts = GST_BUFFER_TIMESTAMP (buf) * 9 / (GST_SECOND / 10000);
    pkt.pts = gst_ffmpeg_time_gst_to_ff (GST_BUFFER_TIMESTAMP (buf),

Created attachment 60413 [details] [review]
fixed up version

Same as latest patch with fixes.

     /* set time */
+    pkt.pts = GST_BUFFER_TIMESTAMP (buf) * 9 / (GST_SECOND / 10000);
Hi,

the following code is from our hacked vesrion. I have included it accidentaly.
The packet timestamping code should not be changed. I am sorry about that

     pkt.pts = gst_ffmpeg_time_gst_to_ff (GST_BUFFER_TIMESTAMP (buf),
-        ffmpegmux->context->streams[bufnum]->time_base);
-    pkt.dts = pkt.pts;
+        ffmpegmux->context->streams[best_pad->padnum]->time_base);
+    if (GST_BUFFER_DATA (buf) != GST_BUFFER_MALLOCDATA (buf)) {
+        pkt.dts = *((GstClockTime *) GST_BUFFER_MALLOCDATA (buf))
+            * 9 / (GST_SECOND / 10000);
+    } else {
+        pkt.dts = pkt.pts;
+    }
+

Just leave
     pkt.pts = gst_ffmpeg_time_gst_to_ff (GST_BUFFER_TIMESTAMP (buf),
         ffmpegmux->context->streams[bufnum]->time_base);
     pkt.dts = pkt.pts;
in the code.

still way too buggy to go in cvs.

I can't manage to get a simple transcoding pipeline to work and there's not enough error information.

Could you please provide an extensive list of pipelines on which it was tested and for which it works for you in order to figure out what's going wrong.

I have forgot one more thing in the patch:
It seems to me that setting time_base at the line 296 is wrong. Timebase should be  initialized by ffmpeg itself. 

I have tested the muxer by transcoding DVDs to mpeg-TS and mpeg-PS (also .vob format) using codecs h264/mpeg2audio and mpeg2video/mpeg2audio. The system was already deplayed to our customer and it works just fine.

Michael, could you please submit a clean updated patch:
_ clean : no weird 'for-your-customer-only' modifications (like in comment #9)
_ updated : against current gst-ffmpeg cvs

btw, aren't you facing legal issues using these mpeg muxers for commercial reasons ?

Created attachment 72415 [details] [review]
ffmux port gst 0.10

This a new patch version. It has been created almost from scratch. I have tested mpeg, mpegts, flv, vob. I have tested with valgrind and found no obvious problems.

The following pipe works without problems

gst-launch -v --gst-debug-level=2 filesrc location=movie.mpeg ! decodebin name=d d. ! ffmpegcolorspace ! ffenc_mpeg2video ! queue ! m. d. ! audioconvert ! lame ! queue ! m. ffmux_mpeg name=m ! filesink location=transcoded.mpeg

[To Edward]: What legal issues do you mean? Do mean patent issues? I thougth that Europe is still patent-safe. FFmpeg is LGPL so there is no problem to use gst-fmpeg plugins in proprietary encoder and of course, we are releasing the sources. BTW: We do not use ffmpegmux anymore.

(In reply to comment #14)
> The following pipe works without problems
> 
> gst-launch -v --gst-debug-level=2 filesrc location=movie.mpeg ! decodebin
> name=d d. ! ffmpegcolorspace ! ffenc_mpeg2video ! queue ! m. d. ! audioconvert
> ! lame ! queue ! m. ffmux_mpeg name=m ! filesink location=transcoded.mpeg
> 

  Excellent, will give it another try and if it doesn't need too much cleaning-up I'll commit it.

> [To Edward]: What legal issues do you mean? Do mean patent issues? I thougth
> that Europe is still patent-safe. FFmpeg is LGPL so there is no problem to use
> gst-fmpeg plugins in proprietary encoder and of course, we are releasing the
> sources. BTW: We do not use ffmpegmux anymore.
> 

  The code is LGPL, but distributing software that implements some patent-protected codecs still requires you to have a license if you want to distribute worldwide (I'm thinking mostly about the mpeg consortium/mp3 licenses). It's not at all sourcecode issue.

(In reply to comment #15)
>   Excellent, will give it another try and if it doesn't need too much
> cleaning-up I'll commit it.

Good, tell if you run into any sort of issue.

> > [To Edward]: What legal issues do you mean? Do mean patent issues? I thougth
> > that Europe is still patent-safe. FFmpeg is LGPL so there is no problem to use
> > gst-fmpeg plugins in proprietary encoder and of course, we are releasing the
> > sources. BTW: We do not use ffmpegmux anymore.
> > 
> 
>   The code is LGPL, but distributing software that implements some
> patent-protected codecs still requires you to have a license if you want to
> distribute worldwide (I'm thinking mostly about the mpeg consortium/mp3
> licenses). It's not at all sourcecode issue.

We are well aware about this and we are aware that some patent licences may be incompatible with (L)GPL. We are going to play the game according to the rules ;-)

latest patch was much cleaner, it's now in cvs. There's still work to do though in gstffmpegcodecmap.c to put proper mappings of supported codecs for each container format.

2006-09-08  Edward Hervey  <edward@fluendo.com>

	Patch by: Michal Benes  <michal dot benes at xeris dot cz>
	
	* ext/ffmpeg/Makefile.am:
	* ext/ffmpeg/gstffmpeg.c: (plugin_init):
	* ext/ffmpeg/gstffmpegcodecmap.c: (gst_ffmpeg_caps_to_codecid):
	* ext/ffmpeg/gstffmpegmux.c: (gst_ffmpegmux_base_init),
	(gst_ffmpegmux_init), (gst_ffmpegmux_finalize),
	(gst_ffmpegmux_request_new_pad), (gst_ffmpegmux_setcaps),
	(gst_ffmpegmux_collected), (gst_ffmpegmux_change_state),
	(gst_ffmpegmux_register):
	Port of FFMpeg muxers to 0.10.
	Still needs some loving in gstffmpegcodecmap to have them all supported
	with correct input formats.
	Closes #332339