[FFmpeg-devel] [PATCH 3/4] oggdec: add support for Opus codec.

[Nicolas George]

Thanks for the comments. Unfortunately, you are referring to the first
version; there was a second version somewhat later:

http://ffmpeg.org/pipermail/ffmpeg-devel/2012-July/127085.html

The major change is that the whole header packet is stored in the codec
extradata; the reason for that is to be compatible with the draft for
MatroskaOpus.

Le tridi 23 messidor, an CCXX, Gregory Maxwell a écrit :
> You MUST read the channel map field to determine if there is a map to
> read, thats what its there for. Don't just guess based on the size.
> If the specification was unclear on this please help me understand
> where it threw you off so it can be fixed.

I will take your this comment into account, but I believe it is a change in
the glue decoder code, not in this parser.

> If you enforce any maximum header size at all it should be conditioned
> on the major version = and the minor version = 0 or 1.  If it turns
> out something important was omitted from the header it may be added in
> a minor version 2 for example.  Reject headers with major version>0 is
> probably a good idea.

My intention was to ignore the version field until a version 2 is defined,
but I can error out on version mismatch too.

> Without reading the skip data you will not be able to get the length
> right, and you will decode incorrectly with and offset potentially
> annoying sounds at the start, you will also get the timestamps wrong.

I know. There is actually some commented-out code taking that into
consideration. Michael is currently working on a general solution for this
kind of padding, I was waiting for it to settle before submitting an updated
version of this patch.

Concerning timestamps, I am completely unsure what to make of them. If the
packets decode to 960 samples, and the first 350 samples must be discarded,
what timestamp should have the second packet? 960 or 960-350=610? And the
first? 0 or -350?

I am also a bit uncertain about that requirement:
"When seeking [...] the decoder should start decoding (and discarding the
output) at least 3840 samples (80 ms) prior to the seek point"

I am not sure lavf is equipped for that. Michael's patches for padding
should be able to handle the 3840 samples, but it is likely to cause A-V
desync with the current code.

> Likewise if you do not read the gain field you will decode files
> incorrectly and they may blast out at loud levels are be nearly
> in-audible.

I must say I am quite baffled by that gain field, it looks like exposing
part of the codec's internal workings, and I see no good reason for it. I
have half a mind of purposefully ignoring it until either all encoders
decide not tu use it or until the API grows some kind of
OPUS_SET_DECODE_GAIN or something.

But I can listen to reason if someone explains why it should be the client
application's task to handle the gain.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120710/a128b4b4/attachment.asc>