GNOME Bugzilla – Bug 343184
[mpeg2enc] ported to 0.10
Last modified: 2006-07-13 11:09:34 UTC
Well, subject says most of it :) The attached patch ports the mjpegtools based mpeg2enc to 0.10. Also, I believe/hope that it satisfies (or is close to it) the requirements to go into good/ugly (for instance, patch includes a unit test). Of course, some further adjustments may be in order (for instance, the name/nick etc for enums), and as such, this is also a request for the appropriate 'sponsoring' :)
Created attachment 66367 [details] [review] Patch porting mpeg2enc Given mjpegtools' mpeg2enc architecture, the patch makes mpeg2enc into sort of a hybrid push based, task based element. Note that the patch also covers modifications to configure.ac, which includes: - a small hack to compensate for mpeg2enc header files containing too many #include config.h - a test for whether mjpegtools 1.6.x or mjpegtools 1.8.0 (or above) is used (could be done in a different way, I suppose, but mpeg2enc headers don't seem to contain any version info) Also note that the stock mjpegtools-1.8.0 does not seem to install the header file mpeg2enc/mpeg2syntaxcodes.h, which gives rise to compiler complaints if not manually catered for.
Cool, you've just made a lot of people very happy ;) However, the test case doesn't seem to compile. I get: make[3]: Entering directory `/home/tim/gst-plugins-bad/tests/check' if gcc -DHAVE_CONFIG_H -I. -I. -I../.. -pthread -I/usr/local/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libxml2 -g -O2 -MT mpeg2enc.o -MD -MP -MF ".deps/mpeg2enc.Tpo" -c -o mpeg2enc.o `test -f 'elements/mpeg2enc.c' || echo './'`elements/mpeg2enc.c; \ then mv -f ".deps/mpeg2enc.Tpo" ".deps/mpeg2enc.Po"; else rm -f ".deps/mpeg2enc.Tpo"; exit 1; fi elements/mpeg2enc.c:55: error: static declaration of ‘mutex’ follows non-static declaration /usr/local/include/gstreamer-0.10/gst/check/gstcheck.h:112: error: previous declaration of ‘mutex’ was here
Created attachment 66384 [details] [review] Patch (additive) for test case Hm, it seems indeed reasonable there are some compiler complaints. Strangely enough (?), mine did not feel like complaining, as it took more doing to get the test running than compiling ;) (given mpeg2enc's hybrid way). Anyway, an additive patch that does some renaming is attached.
Created attachment 66419 [details] [review] additional patch for ext/Makefile.am ext/Makefile.am needs patching too. > Also note that the stock mjpegtools-1.8.0 does not seem to install > the header file mpeg2enc/mpeg2syntaxcodes.h, which gives rise to > compiler complaints if not manually catered for. What do you mean by 'if not manually catered for'? How do you cater for it? With your patch mpeg2enc compilation on my dapper system (and libmjpegtools 1.8.0) fails because of 'ratectl.hh: mpeg2syntacodes.h - no such file or directory' for me (didn't notice before because of the above ;)) Was your patch supposed to include that file? (and is upstream aware of this bug?)
> ext/Makefile.am needs patching too. Oops, indeed, probably overlooked this somehow because mine also includes a change for mplex ;) > > Also note that the stock mjpegtools-1.8.0 does not seem to install > > the header file mpeg2enc/mpeg2syntaxcodes.h, which gives rise to > > compiler complaints if not manually catered for. > > What do you mean by 'if not manually catered for'? How do you cater for it? I got the problem above as well; compilation (at least gcc3 ;)) succeeds after: - a bit of a hack in configure.ac (which is included in the patch) - copying the mpeg2enc/mpeg2syntaxcodes.h in the 1.8.0 tarball to the include dir (e.g) /usr/[local/]mjpegtools/mpeg2enc > With your patch mpeg2enc compilation on my dapper system (and libmjpegtools > 1.8.0) fails because of 'ratectl.hh: mpeg2syntacodes.h - no such file or > directory' for me (didn't notice before because of the above ;)) Agreed, the above 'catering' should help :) > Was your patch supposed to include that file? It is part of mjpegtools, so I suppose not (??) [the problem in mjpegtools is that either it is mentioned as an 'include' too many (in ratectl.hh), or classifying it as noinst_HEADERS is a slip] However, I was/am planning to come up with a patch for this problem (and the 'config.h' one) and put it here (and/or sourceforge, see below). > (and is upstream aware of this bug?) If by 'upstream' is meant the mjegtools project (maintainers), then yes. At least, the mpeg2syntaxcodes.h thingy is entered as a bug on sourceforge. Fix seems to be planned as part of a (larger) header repartitioning (work in progress), but a small fix/patch (if possible) in the meantime may be helpful (for smoother sailing in compilation at least ;) ).
> > Was your patch supposed to include that file? > It is part of mjpegtools, so I suppose not (??) No, indeed it shouldn't. I just misunderstood what you meant earlier on. My bad. I was under the impression you were aware of that problem and your patch already included a work-around for it. > However, I was/am planning to come up with a patch for this problem (and the > 'config.h' one) and put it here (and/or sourceforge, see below). Great. I don't think we can ship a plugin (and enable it) that doesn't compile out of the box with the headers that are provided by upstream/packages in the common distros ... > > (and is upstream aware of this bug?) > If by 'upstream' is meant the mjegtools project (maintainers), then yes. > At least, the mpeg2syntaxcodes.h thingy is entered as a bug on sourceforge. > Fix seems to be planned as part of a (larger) header repartitioning (work in > progress), but a small fix/patch (if possible) in the meantime may be helpful > (for smoother sailing in compilation at least ;) ). Shame, it would be much easier if they (mjpegtools) just did a quick 1.8.1 brown paper bag release.
Alright, finally committed: 2006-07-13 Tim-Philipp Müller <tim at centricular dot net> Patch by: Mark Nauwelaerts <manauw at skynet be> * configure.ac: * ext/Makefile.am: * ext/mpeg2enc/Makefile.am: * ext/mpeg2enc/gstmpeg2enc.cc: * ext/mpeg2enc/gstmpeg2enc.hh: * ext/mpeg2enc/gstmpeg2encoder.cc: * ext/mpeg2enc/gstmpeg2encoder.hh: * ext/mpeg2enc/gstmpeg2encoptions.cc: * ext/mpeg2enc/gstmpeg2encpicturereader.cc: * ext/mpeg2enc/gstmpeg2encpicturereader.hh: * ext/mpeg2enc/gstmpeg2encstreamwriter.cc: * ext/mpeg2enc/gstmpeg2encstreamwriter.hh: Port mpeg2enc to 0.10 (#343184). * tests/check/Makefile.am: * tests/check/elements/.cvsignore: * tests/check/elements/mpeg2enc.c: Add unit test for mpeg2enc. * tests/icles/.cvsignore: Ignore pitch-test. Thanks for the patch! I've added check for mpeg2syntaxcodes header to configure.ac check and removed the superfluous(?) mplex stuff in there, hope that works as intended.