[vorbis-dev] my vorbis comments

[Michael Smith]

At 11:51 PM 9/13/00 +0900, you wrote:
>
>Ok I just finished writing a Vorbis encoder/decoder, and here are some
>of my comments (I'm sure some of these are well known, but here they
>are anyway - sorry this is long):
>
>1. vorbis_info:
>  a. Accessing the pre-computed info structs through pointers to
>     static data is messy IMO.  I should be able to vorbis_info_init
>     (or similar) an info struct to be exactly the same as info_A, for
>     instance.  That way, I could vorbis_clear and free it at the end
>     just like I do for vorbis_info's which come from a vorbis file

There's going to be a bunch of API additions related to mode selection -
the current method is basically just a temporary hack. This applies to a
number of your other encode-side comments below.

>  b. A function which compares two vorbis_info structs, to see if they
>     are equivalent (i.e. would produce the same output for a given
>     input stream) This would be useful say if I am copying an input
>     vorbis file (whose info is not info_[A-E]) to an output vorbis
>     file with a specific info, I could determine then whether it is
>     necessary to actually re-encode, or whether I could just copy
>     packets.

Is this really useful? If you're doing anything which potentially involves
re-encoding, the chance of having an identical output info struct is almost
zero. Also, if you're encoding, the output info struct contains more
information than the one you've just read from a file - so even if the
common elements are the same, it's possible that the result will be quite
different.

>  c. A way to cleanly write a file using the same info struct I pulled
>     from an input vorbis file.  vorbiscomment.c, of course, has this
>     same problem, and so I had to borrow the hack from there.

Yes. This basically needs the header reading/writing functions to be split
up (or at least that's the straightforward way). Vorbiscomment would
ideally not be using ANY vorbis_* functions (just the ogg functions) except
the comment functions.

>  d. People say the info_[A-E] has approximate bitrate X, but those
>     structs don't have the bitrate_* fields set, what's up with that?

The bitrate_* fields are for CBR (and constrained VBR). So they aren't used
yet (you'll see that the library doesn't use them anywhere, except to write
and read them). The approximate bitrates are a function of the
psychoacoustic sets and the codebooks used, at the moment. There's no
simple way to figure out what they are just from looking at them, which is
why the headers say what they are.

>  e. The other stuff I'm _sure_ you folks are already working on, for
>     example:
>    i. A more general way to choose an info struct

Yes, as mentioned above.

>2. vorbisfile:
>  Write capability.

I think there's a encode-side equivalent to vorbisfile intended (but not
incorporated into vorbisfile itself).

>3. ogg_page:
>  A way to compute page length.  In order to compute the page length,
>  I've been adding header_len and body_len - which is currently
>  correct - but it's not part of the spec as far as I can tell.
>  ogg_sync_pageseek would tell me the page length, but if I should use
>  that, then who needs ogg_sync_pageout?

You don't, really. ogg_sync_pageout() is easier to use if you lose sync at
some point (it internally calls ogg_sync_pageseek() anyway.)
Perhaps ogg_sync_pageout() should return total page size instead of 1 in
case of success?

>4. analysis/synthesis:
>  a. I wouldn't mind if vorbis didn't use separated channel buffers,
>     but since I don't understand the compression details, they might
>     be really important.

Not entirely sure what you mean, here. Can you clarify? If you're talking
about how vorbis uses seperate buffers for each channel, rather than a
single buffer with the data interleaved, then there's a good reason for it
- it makes the internals a lot nicer. Would be very hard to change.

>  b. I also wouldn't mind if there were vorbis_read_header
>     /_write_header functions which handled the first 3 packets for
>     me.

I agree, the header bits of the api need changes.

>5. chained streams:
>  a. The fact that vorbis allows different sampling-rate sounds to be
>     chained together makes my head want to explode.

You should just treat chained streams as a set of completely independent
streams (which they are). So the different sample-rates shouldn't be a
problem.  I believe mp3 allows sample rate changes mid-stream (though I may
be wrong here, and I don't know of anything that correctly supports this) -
so merely having chained streams that can be completely different isn't
nearly as bad...

>
>Finally, this last note could be a bug in my code, I have not finished
>looking into it.  But I thought I'd mention it.  Feel free to ignore
>from here on.
>
>  When I read the first three packets (the headers) of a vorbis file,
>  I expect that when I am finished reading the last packet, the offset
>  into the stream (as computed by adding the page sizes together)
>  should be the offset of the first sound page.  I don't know if
>  that's spec, but it's what vorbisfile does, so I took it as gospel.
>  However, when _my_ program does it, I end up needing 1 more page
>  than I should for that last packet.  That is, I have to _pagein the
>  first sound page before I can _packetout the last header packet.

I'm not sure I understand what you've been doing here, but it sounds like
you're expecting the current page to end after the last header (such that
the audio starts on a new page). This indeed should be the case (after some
discussion a week or so back). 

However, if you based your encoder off any of the sample code more than a
week ago, then it wouldn't be doing this (flushing pages after the
headers). You should make this change, but you might also want to deal
correctly with files which don't do this.

Michael

--- >8 ----
List archives:  http://www.xiph.org/archives/
Ogg project homepage: http://www.xiph.org/ogg/
To unsubscribe from this list, send a message to 'vorbis-dev-request at xiph.org'
containing only the word 'unsubscribe' in the body.  No subject is needed.
Unsubscribe messages sent to the list will be ignored/filtered.