[Vorbis-dev] Patch to stop vcut from generating broken streams

[Michael Vrable]

<200406211141.51197.msmith at xiph.org>
Message-ID: <20040621055300.GA15022 at vrable.net>

On Mon, Jun 21, 2004 at 11:41:51AM +1000, Michael Smith wrote:
> On Saturday 19 June 2004 12:12, Michael Vrable wrote:
> > The attached patch is intended to fix the bug in vcut where the
> > granulepos on each page is not always set correctly in the output
> > stream.  (See, for example, the discussion on vorbis-dev at xiph.org in
> > August 2003, "vcut breaks song index ? XMMS search fails".)  It's an
> > issue I came up against while editing the recording of a webcast I
> > helped organize; I created a workable fix then, and I've tried to clean
> > it up now.
> >
> > The patch should be against the most recent version of vcut from
> > svn.xiph.org (not that it seems to be undergoing much active
> > development).
>
> Well, yes. vcut isn't built by default, and I've been suggesting for the past
> year or so that nobody should ever use it. It was written as proof-of-concept
> code, and it successfully proved the concept. It isn't intended for actual
> use. The fact that it went into vorbis-tools was, in retrospect, a mistake.

That said, it _is_ a handy tool to have around.  I'm not currently aware
of another tool that allows for lossless cutting of a Vorbis stream (I'd
love to be pointed to one).  It was invaluable for some editing I did at
one point, and at least until a good replacement for it exists, it would
be nice to have a (mostly) working version of vcut for when it's needed.

> > I tried to stick to the original coding style as closely as I could.
> > (Including what appeared to be 4-space tabs(!).)
>
> They're the only reasonable way to do tabs! :-) (oh, and you missed "80 column
> lines", which isn't stuck to as closely as it should be in the existing
> source, but is intended...)

(I generally stick with 4-space indentation but 8-space tabs in code I
write.  Your code did look a lot better once I realized tabs were
supposed to be every four spaces and adjusted my editor accordingly. :-)
I mostly tried to stick to 80-column lines, but gave up on some long
strings, as I noticed had been done elsewhere.  I could try to clean
that up a bit more.)

> Whilst I expect (without having actually looked at it yet) that this patch
> drastically improves things, I don't really want to do anything to vcut that
> might suggest to people that it's actually considered _usable_ (because I
> don't, even with these fixes, due to a) and b) below.)
>
> Well... unless you actually make it work properly in all cases - at the very
> least this requires heavy testing on streams with a) the start cut off, as
> you described, and b) chained streams. Neither of which currently work.

I believe, not having tested this, that the current behavior on streams
that start with a positive granulepos is to cut at the indicated
location as measured against the granulepos values in the stream.  So
cutting a stream at 35 seconds when the stream starts at 30 seconds will
actually cut after only 5 seconds of audio.  This may or may not be what
the user expects.  It shouldn't be too difficult to always measure from
the first audio sample contained in the stream.  (This change would be
made if adding support for chained bitstreams, at least as I imagine
it.)

With regards to chained streams: I have thus far been treating that as a
"don't do that!" case (though I don't think there are any checks for
this in the code).  To actually handle chained streams properly requires
first defining how they _should_ be treated.  I'm thinking:

Imagine decoding the entire physical input bitstream, concatenating
the samples decoded from each logical bitstream.  Seek through the
specified number of samples within this imaginary output stream,
determine at which position within which logical stream in the input
that sample corresponds, and make the cut there.

Are there other reasonable ways to define this?

Here, we are completely blind to the timing information conveyed by
streams that start at a positive granulepos; we only care about the
number of samples of audio actually encoded in each stream.  The
granulepos values in the input stream only matter when they indicate
that some decoded samples are to be discarded.

Complications:

- We can't immediately copy the headers from the start of the input
stream to the second output stream, since the cutpoint might
actually be in a subsequent stream.  Not too difficult.

- Almost anything can change midway in a chained stream--even the
sampling rate.  If the cutpoint has been specified in samples, print
a warning (but do allow the user sample-precise measures, since this
could still be useful).  If the cutpoint has been specified in
seconds, more logic is needed to count the time properly (we can't
simply multiply by the sampling rate at the beginning as is done
currently).

- There would still be some unhandled cases.  Cutpoints within two
packets of the start or end of a stream will cause trouble.  Such a
cutpoint require generating an output stream with only two packets,
and I don't believe there's a way to specify how many samples to
discard from the start and the end in a stream this short.

In this case, print an error message and abort.  Possibly permit
discarding the short output stream, or recommend how to shift the
cutpoint to make the cut possible.

- Other issues that I'm forgetting?

It might also be worth giving the user the option of adjusting the
cutpoint to the nearest packet boundary.  I can see this as useful when
sample-precise editing isn't necessary.

I'm willing to give coding this a shot.  It would take a bit longer, but
looks doable.  (The previous patch was a one-night hack, though that was
with the benefit of having previously coded then lost the sources for a
similar fix.)

Thanks for the comments.  I'm open to more suggestions and advice (from
you, or from other people with an interest in vcut).

--Michael Vrable