[Tremor] Some issues with Nicholas' comment truncation patch

[Andy Grundman]

Right now I'm looking at using Nicholas' patch and the following code  
in _vorbis_unpack_comment.  The IP3K define is for code that's been  
added.  This is just a work-in-progress, my eventual plan is to define  
another static memory area for comment storage and read as many  
comments as I can fit into that space, and skip everything after the  
limit has been reached using a new function _v_skipstring.  We have a  
large amount of external SDRAM we can use for this purpose, rather  
than calling malloc/calloc which would store the comments in a much  
smaller memory region.

I originally just wanted to skip all comments but I'm also working on  
fixing our handling of chained bitstreams and in order to support  
metadata from Ogg radio streams, I need to read in the comments.  Most  
radio stations only send a small number of comments, so if I limit  
comments to something like 512 bytes or even less I should be fine.

#ifdef IP3K
static void _v_skipstring(oggpack_buffer *o,int bytes){
   while(bytes--){
     oggpack_read(o,8);
   }
}
#endif

...

   for(i=0;i<vc->comments;i++){
#ifdef IP3K
     if(has_been_trunc && opb->headend < 4)
       break;
#endif
     int len=oggpack_read(opb,32);
#ifdef IP3K
     if(has_been_trunc && opb->headend < len+1)
       break;

     // Ignore comments that are > 128 chars
     if(len > 128) {
       DEBUG_TRACE("skipping comment of length %d", len);
       _v_skipstring(opb,len);
       vc->comment_lengths[i]=0;
       vc->user_comments[i]=(char *)_ogg_calloc(1,1); // XXX
       continue;
     }
#endif
     if(len<0)goto err_out;
     vc->comment_lengths[i]=len;
     vc->user_comments[i]=(char *)_ogg_calloc(len+1,1);
     _v_readstring(opb,vc->user_comments[i],len);
   }

On Jan 22, 2009, at 3:30 PM, Ethan Bordeaux wrote:

> FYI I'm also looking at this problem right now.  I too need to be  
> able to ignore comments of arbitrarily large size.  The problem that  
> I have is that I've currently statically allocated for a maximum of  
> 7 ogg buffers in _fetch_buffer().  This works fine for small  
> comments (and all decoding needs I've run into thus far) but large  
> comment headers make additional requests to get more ogg buffers.  I  
> think that in the comment decode phase Ogg will allocate buffers  
> continuously to hold all the metadata, which I can't handle.
>
> My thought was to short-circuit this behavior so that, if we're in  
> the comment gathering phase, all data reads go to a dummy  
> ogg_buffer.  I would probably add the code in ogg_buffer_alloc() to  
> check if this is a real request for a new buffer or if we should map  
> to the dummy buffer/reference.  I'm pretty sure I can safely find  
> the start and end points of this phase by setting a flag right after  
> _vorbis_unpack_info finishes and clearing the flag after  
> _vorbis_unpack_comment finishes. This is a pretty big kludge but I  
> think it would work.
>
> Thoughts?
>
> On Thu, Jan 22, 2009 at 10:28 AM, Andy Grundman  
> <andy at slimdevices.com> wrote:
> OK, I need to make some more test files and see if any of them can be
> read properly.  I may use your patch anyway because it at least avoids
> allocating tons of memory and crashing.  Even if the file doesn't play
> it will fail more gracefully than it currently does.
>
> On Jan 22, 2009, at 10:18 AM, Nicholas Vinen wrote:
>
> >
> > Unfortunately while in many cases the patch works, I've found that  
> it
> > has problems with certain files. It's a bit of a hack and I don't  
> yet
> > fully understand why it doesn't work.
> >
> > A much better solution would be to examine the header of any packet
> > which exceeds a certain size and is not yet fully read in yet, and
> > determine the type. If it's the comment field, it should be possible
> > to
> > process what's already been read in and sync up to the next packet
> > without reading the rest in at all, rather than what I did which  
> is to
> > prevent the current packet in memory growing once it reaches a
> > certain size.
> >
> > So in summary I think somebody needs to come up with a better
> > solution.
> > I will do it at some point but I don't know when that will be, it
> > could
> > be a while. Someone who understands the Ogg packet code better would
> > likely do a good job. I don't yet understand it fully.
> >
> > You may be able to get my code to work but as it is unfortunately I
> > don't think it's 100%. And yes you may be right, the later patch may
> > have broken the large comment skipping.
> >
> >
> > Nicholas
> >
> >
> >
> > Andy Grundman wrote:
> >> Hi Nicholas,
> >>
> >> I'm working on implementing your comment truncation patch into our
> >> Tremor code.  I have a few questions about it.
> >>
> >> Comparing Tremor3.patch [3] and your original patch [1], #3 seems  
> to
> >> have removed the extra code in _vorbis_unpack_comment that breaks  
> out
> >> of the comment reading loop if the data was truncated.  Using patch
> >> #3
> >> this loop always tries to allocate space for the truncated comment
> >> and
> >> fails, so I think what you did in #1 is the right solution.   
> However,
> >> using #1 I end up failing on my test file with ENOTVORBIS because  
> the
> >> check for the "vorbis" string in vorbis_dsp_headerin fails when
> >> looking for the codebooks header.
> >>
> >> Thanks,
> >> -Andy
> >>
> >> [1] http://x256.org/~hb/Tremor.patch
> >> [2] http://x256.org/~hb/Tremor3.patch
>
> _______________________________________________
> Tremor mailing list
> Tremor at xiph.org
> http://lists.xiph.org/mailman/listinfo/tremor
>