ogg dev - Nov 2010 - Fwd: Merging jorbis upstream and the cortado jorbis fork back into one

Of interest to some on the list.

Anyone familiar enough with Java to know how we go about
detecting/using/incorporating an external Jorbis build into the
Cortado jar ?
Or are we supposed to download sources into our tree and build the whole ?


---------- Forwarded message ----------
From: Hans de Goede <hdegoede at redhat.com>
Date: Thu, 04 Nov 2010 15:31:31 +0100
Subject: Merging jorbis upstream and the cortado jorbis fork back into one
To: ymnk at jcraft.com, jorbis at jcraft.com, ogg.k.ogg.k at googlemail.com,
bens at alum.mit.edu, twerner at debian.org, onkarshinde at ubuntu.com
Cc: freecol-developers at lists.sourceforge.net

Hi All,

I'm sending you this mail because you're all somehow involved in
jorbis and / or cortado development.

Short self-intro: I've been a FOSS developer for over a decade and amongst
other things I maintain a ton of packages in Fedora.

While updating Fedora's freecol package to the latest upstream I noticed
that it had grown a dependency on cortado. So I started looking into
packaging cortado for Fedora.

I soon found out that cortado contains its own copy of the jorbis sources.

After much diff-ing between upstream jorbis releases and cortado's jorbis
code (made harder by the fact that older jorbis releases cannot be
downloaded). I've come to the conclusion that the jorbis copy in cortado
was forked of from jorbis upstream at 0.0.12. This means that
the cortado bundled copy is missing the following bugfixes / improvements:

Changes since version 0.0.16:
- change: refactoring code to improve the performance;
           suppressing frequent GC in Residue.

Changes since version 0.0.15:
- feature: added a property 'jorbis.player.playonstartup' to
JOrbisPlayer
            applet to play given stream at the start-up time.
            Refer to 'play/JOrbisPlayer.html'.

Changes since version 0.0.14:
- bugfix: case-insensitive matching in Comment was broken.
- bugfix: query for the tag "ART" would match the tag
"ARTIST".

Changes since version 0.0.13:
- added com.jcraft.jogg.Buffer.readB method for Fluendo's theora decoder.
- added com.jcraft.jogg.Page.copy method for handling grouped ogg streams.

Changes since version 0.0.12:
- fixed resorce leaks in com.jcraft.jorbis.VorbisFile class.


Note that the:
- added com.jcraft.jogg.Buffer.readB method for Fluendo's theora decoder.
change is present in cortado's copy, Since upstream jvorbis promptly
picked this up I wonder why cortado is carying its own fork.

Other then missing all the above changes since jorbis-0.0.12 cortado's
copy does contain 2 bugfixes not present in upstream jorbis.

I've attached a patch with these 2 bugfixes to this mail. Note that this
patch has the original cortado git headers for reference, but the actual
patches / diffs are not directly from cortado's git. They have been
rebased to apply to jorbis-0.0.17

People form jcraft, could you perhaps do a new upstream release including
these 2 fixes ?

People from Debian, for Fedora's packages I've used the upstream jorbis
+
the attached patch, removing the included copy from cortado. AFAIK Debian's
policy also disallows shipping embedded copies, so I think you should do the
same for Debian.

People from cortado, as the long list of missing upstream fixes (and users
of upstream jorbis missing your bugfixes) shows, having a forked copy
inside your tree is a bad idea, can you please stop doing that ?

Thanks & Regards,

Hans
-------------- next part --------------
From: Maik Merten <maik at maik-desktop.(none)>
Date: Wed, 28 Oct 2009 18:50:41 +0000 (+0100)
Subject: apply a patch written by Philip Heron: "This fixes a bug where
Cortado would die...
X-Git-Tag: 0.5.1~21
X-Git-Url:
http://git.xiph.org/?p=cortado.git;a=commitdiff_plain;h=08bad890e3ff664fbb78dcb1c640bc739445369e

apply a patch written by Philip Heron: "This fixes a bug where Cortado
would die or display corrupt video when a Theora stream begins with an
incomplete packet."

ticket #1565
---

diff -up jorbis-0.0.17/com/jcraft/jogg/StreamState.java~
jorbis-0.0.17/com/jcraft/jogg/StreamState.java
--- jorbis-0.0.17/com/jcraft/jogg/StreamState.java~	2008-05-07
10:07:39.000000000 +0200
+++ jorbis-0.0.17/com/jcraft/jogg/StreamState.java	2010-11-04 14:09:06.019015004
+0100
@@ -294,10 +294,13 @@ public class StreamState{
         lacing_vals[lacing_fill++]=0x400;
         lacing_packet++;
       }
+    }
 
-      // are we a 'continued packet' page?  If so, we'll need to
skip
-      // some segments
-      if(continued!=0){
+    // are we a 'continued packet' page?  If so, we'll need to skip
+    // some segments
+    if(continued!=0){
+      if(lacing_fill<1 ||
+         lacing_vals[lacing_fill-1]==0x400){
         bos=0;
         for(; segptr<segments; segptr++){
           int val=(header_base[header+27+segptr]&0xff);
From: Gregory Maxwell <greg at xiph.org>
Date: Fri, 19 Feb 2010 04:45:21 +0000 (-0500)
Subject: Fix jorbis for surround sound support. No fancy downmixing yet.
X-Git-Tag: 0.6.0~30
X-Git-Url:
http://git.xiph.org/?p=cortado.git;a=commitdiff_plain;h=c1d9cef5129e0b452375d7da9129dad435d1a918

Fix jorbis for surround sound support. No fancy downmixing yet.
---

diff -up jorbis-0.0.17/com/jcraft/jorbis/Mapping0.java~
jorbis-0.0.17/com/jcraft/jorbis/Mapping0.java
--- jorbis-0.0.17/com/jcraft/jorbis/Mapping0.java~	2008-05-07 10:06:50.000000000
+0200
+++ jorbis-0.0.17/com/jcraft/jorbis/Mapping0.java	2010-11-04 14:14:30.774014847
+0100
@@ -99,8 +99,8 @@ class Mapping0 extends FuncMapping{
       opb.write(1, 1);
       opb.write(info.coupling_steps-1, 8);
       for(int i=0; i<info.coupling_steps; i++){
-        opb.write(info.coupling_mag[i], Util.ilog2(vi.channels));
-        opb.write(info.coupling_ang[i], Util.ilog2(vi.channels));
+        opb.write(info.coupling_mag[i], Util.ilog2roundup(vi.channels));
+        opb.write(info.coupling_ang[i], Util.ilog2roundup(vi.channels));
       }
     }
     else{
@@ -136,8 +136,8 @@ class Mapping0 extends FuncMapping{
       info.coupling_steps=opb.read(8)+1;
 
       for(int i=0; i<info.coupling_steps; i++){
-        int testM=info.coupling_mag[i]=opb.read(Util.ilog2(vi.channels));
-        int testA=info.coupling_ang[i]=opb.read(Util.ilog2(vi.channels));
+        int
testM=info.coupling_mag[i]=opb.read(Util.ilog2roundup(vi.channels));
+        int
testA=info.coupling_ang[i]=opb.read(Util.ilog2roundup(vi.channels));
 
         if(testM<0||testA<0||testM==testA||testM>=vi.channels
             ||testA>=vi.channels){
diff -up jorbis-0.0.17/com/jcraft/jorbis/Util.java~
jorbis-0.0.17/com/jcraft/jorbis/Util.java
--- jorbis-0.0.17/com/jcraft/jorbis/Util.java~	2008-05-07 10:01:31.000000000
+0200
+++ jorbis-0.0.17/com/jcraft/jorbis/Util.java	2010-11-04 14:14:08.039015001
+0100
@@ -19,6 +19,16 @@ class Util{
     return (ret);
   }
 
+  static int ilog2roundup(int v){
+    int ret=0;
+    if (v>0)v--;
+    while(v>0){
+      ret++;
+      v>>>=1;
+    }
+    return (ret);
+  }
+
   static int icount(int v){
     int ret=0;
     while(v!=0){

On Thu, Nov 4, 2010 at 11:01 AM, ogg.k.ogg.k at googlemail.com
<ogg.k.ogg.k at googlemail.com> wrote:
> Of interest to some on the list.
>
> Anyone familiar enough with Java to know how we go about
> detecting/using/incorporating an external Jorbis build into the
> Cortado jar ?
> Or are we supposed to download sources into our tree and build the whole ?
Cortado should remain self contained.

I would prefer that distributions not package cortado.  In
_particular_ if they're going to do things like replace chunks of it
without testing on things like JVM 1.3. (And, if they are in fact
testing on JVM 1.3 I hope that they come join the upstream team!)

Sorry, forgot to CC the original poster. Now fixed.

On 11/4/10, Gregory Maxwell <gmaxwell at gmail.com>
wrote:
> On Thu, Nov 4, 2010 at 11:01 AM, ogg.k.ogg.k at googlemail.com
> <ogg.k.ogg.k at googlemail.com> wrote:
>> Of interest to some on the list.
>>
>> Anyone familiar enough with Java to know how we go about
>> detecting/using/incorporating an external Jorbis build into the
>> Cortado jar ?
>> Or are we supposed to download sources into our tree and build the
whole ?
>
> Cortado should remain self contained.
>
> I would prefer that distributions not package cortado.  In
> _particular_ if they're going to do things like replace chunks of it
> without testing on things like JVM 1.3. (And, if they are in fact
> testing on JVM 1.3 I hope that they come join the upstream team!)
>

On Thu, Nov 4, 2010 at 11:46 AM, Hans de Goede <hdegoede at redhat.com>
wrote:
> Note that I'm not packaging it (advertising the package) as an applet
> for in browser use / for serving over http, but rather packaging
> it as a java media playback framework, because it is used as such by
> several java games which we package. As such it only gets used
> with (and has been tested with) the jre which we ship.
>
> Also I've done a full diff between the jorbis upstream sources and
> the ones shipped with cortado and they are as good as entirely the
> same except for a few bugfixes in jorbis upstream which are not
> in cortado and visa versa. Atleast cortado should rebase (replace)
> its copy with the latest upstream jorbis release + the patch
> I attached to my original mail. This will fix a resource leak
> in VorbisFile, as well as 2 bugs in the tag handling code.
Hi Hans, the fact that you're packaging it for games not applet use
addresses my concern. Thank you, and thanks for your efforts here.  I
believe debian actually packages it as an applet.

You should be aware that cortado targets a wider set of JVMs than
Jorbis. In particular Cortado will run on the old MS JVMs that jorbis
will not (unless this has changed and it wasn't mentioned in the
jorbis changelog).  Syncing up with jorbis fixes sounds like a great
idea, but it's going to at least require some basic testing in old
JVMs, which is always a bit of a pain.

Someone also ought to perform a sweep of the recent libvorbis changes?
the Jorbis stuff was a rather literal port of the C code and probably
has some of these bugs in common.

Hans de Goede <hdegoede at redhat.com> wrote:
> I've attached a patch with these 2 bugfixes to this mail. Note that
this
> patch has the original cortado git headers for reference, but the actual
> patches / diffs are not directly from cortado's git. They have been
> rebased to apply to jorbis-0.0.17
On further review of the patch I believe it may have highlighted an
additional logic error in jorbis.

The libvorbis code contains only one unique ilog2 function:

static int ilog2(unsigned int v){
  int ret=0;
  if(v)--v;
  while(v){
    ret++;
    v>>=1;
  }
  return(ret);
}

The same code is duplicated in several places.

This was incorrectly ported to java, due to java requirements about
conditionals, as:

private static int ilog2(int v){
  int ret=0;
  while(v>1){
    ret++;
    v>>>=1;
  }
  return(ret);
}

In Mapping0.java I corrected this to:

private static int ilog2(int v){
  int ret=0;
  if (v>0)v--;
  while(v>0){
    ret++;
    v>>>=1;
  }
  return(ret);
}


But I didn't realize that there were other copies of this function in
DspState.java, Floor1.java, and Info.java  (matching the duplication
that occurs in libvorbis).  I suspect that these need all be using the
'fixed' function.

Most (all?) of this time this function is used its used to find the
minimum bits required to code some range, so the upward rounding is
required. (especially the floor1 instance).

I've added looking into this to my todo, but it may take me a while to
get around to it.