[Tremor] Some issues with Nicholas' comment truncation patch

[Nicholas Vinen]

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