Created attachment 13317 [details] [review]
patch for tremor plugin

Hi,

this looks great to have !
I'd love it if we could integrate is.

I looked over the patch, and have a few comments:

a) use of config.h.in is deprecated, so you shouldn't have to patch
that.  I will look at removing that definitively.
b) your code looks like a copy/paste of vorbisfile, so I assume that
the tremor interface is very similar to it.
However, all of the symbols need renaming so they don't clash with
vorbisfile.  Do you think you could do that and resubmit the patch so
that it doesn't clash ?

I could do it myself, but the patch will be integrated a lot quicker
if it doesn't clash :)

Thanks for doing this !

Thomas

Created attachment 13665 [details] [review]
updated patch

I hope to target this for the 0.6.1 release.

tried commiting to head and got this running autogen.sh:
./configure: line 19939: syntax error near unexpected token
`HAVE_IVORBIS=yes,'
./configure: line 19939: `  XIPH_PATH_IVORBIS(HAVE_IVORBIS=yes,
HAVE_IVORBIS=no)

Ok, think I fixed it. I used the tarkin plugin as example and changed 
the configure.ac stuff to the following:

dnl *** ivorbis ***
dnl AM_PATH_IVORBIS only takes two options
translit(dnm, m, l) AM_CONDITIONAL(USE_IVORBIS, true)
GST_CHECK_FEATURE(IVORBIS, [integer vorbis plug-in], ivorbisdec, [
 HAVE_IVORBIS="yes"
])

ok, new issue:
vorbis.c:22:33: tremor/ivorbiscodec.h: No such file or directory
vorbis.c:23:32: tremor/ivorbisfile.h: No such file or directory

Seems some header files needs to be inlcuded in the patch.

Some update. As can be seen from my previous entries I tried applying
this. The fix I attempted was based on an assumption that this plugin
did not have an external dep. It does, and the auto* fix has to
reflect that. 

Moved to 0.6.x because it's a non-ABI enhancement, but not 0.6.0
because we're not wanting to do this in the next 2 days <g>

Since all these bugs are listed as needing to be fixed in 0.6.x, they are by
definition bugs in 0.6.0

Created attachment 14313 [details] [review]
updated patch

ok, plugin integrated into CVS head (0.7 branch), it will migrate from
there into stable eventually. The latest patch contained only build
setup so I had to mix it with the first to get everything working.
Closing this bug now. Thanks for the plugin Phil!

Re-opening bug. The ivorbis auto* files still seem to think I have
tremor installed when I just have standard vorbis installed. I had to
disable building of the tremor plugin until that issue is resolved,
the code is still in CVS head however.

Created attachment 14667 [details] [review]
additional patch

ok, patch merged into head and things work properly now. Thanks Phil!