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

[Nicolas George]

Le quartidi 24 messidor, an CCXX, Gregory Maxwell a écrit :
> There is a zero and one defined though the change there was not of
> great consequence:
> it's what defined the interpretation of the version field.
> 
> The specification splits the version into major and minor fields.
> Implementations
> are supposed to accept all with the 0 major. There might be other fields added
> in other minor versions but they're guaranteed to be compatible though
> the header
> obviously not have the same size.
> 
> If the major version is different all bets are off and you might just
> be producing gibberish
> output. The spec recommends you reject files with major != 1.

Thanks for the clarification, although I still see a contradiction:
according to the spec, the version field "MUST always be '1'", and as I read
the rest, that means major=0 and minor=1. The example with 15 (accepted) and
16 (rejected) confirm that. But you wrote about major!=1: is it a typo from
you or something else?

> Yes, the first samples out of the decoder will have negative time.

Ok, thanks for the authoritative answer.

> If it causes AV desync then it sounds like the timestamps aren't correct.

This is not a problem with Opus, this is a problem I fear with the ffmpeg
command-line tool: I have noticed at a time that audio timestamps were often
neglected. There will be need for careful checks.

> The header is supported in all existing decoders that I'm aware of
> (Opusdec, gstreamer, Firefox).
> 
> The support requires on the order of two lines of code in typical software.
> 
> Ffmpeg would be the odd man out to not support it, and you'll hurt
> users because they won't be able to safely do recodeless gain tweaks
> (like they can with mp3gain and aacgain) because they won't be able to
> trust that the modifications will be applied in all the popular
> software.
> 
> Please don't do this.
> 
> I'll be happy to make whatever concession I can to avoid the
> gratuitous incompatibility.
> 
> I can't adequately express how disappointed I am to see this kind of
> response. We've had years of open development of Opus with basically
> no feedback from this community, though we extended invitations a few
> times— although I do understand everyone is busy with their own
> projects. This header was initially specified over a year ago, and
> there have been pre-release tools implementing it for just as long.
> We've spent the last five years writing tens of thousands of lines of
> code, spent countless hours in negotiation with traditionally less
> open organizations, performing seemingly endless listening tests, and
> authored a whole codec design and specification that we've shared with
> you freely. And at the same time I'm gladly offering to provide review
> and QA for your implementation and I took the time to seek you out in
> order to do so… and you'd break compatibility just to make a point
> over a couple lines of caller implementation complexity?
> 
> If you would like a decoder CTL you're welcome to one— and you don't
> have to threaten to spread incompatible software, you could have just
> asked.

Please forgive me, I spoke way too harshly.

I was somewhat annoyed by the amount of work expected from the API users.
Every codec has its quirks, but I feel it is the task of the high-level API
to hide them as much as possible by default.

That little annoyance added to the big irritation I have for the Ogg format
since I actually looked what it was made of and became the water drop that
broke the CaML's back or something. So please again accept my apologies.

> As you requested, I have added OPUS_SET_GAIN CTL to libopus:
> 
> https://git.xiph.org/?p=opus.git;a=commit;h=28b41ae5ae1acf6eb222858567952564838d48f5

Thanks a lot. But looking at the diff, it looks like it was done as
post-processing, exactly like an application could have done: I am a bit
disappointed because I hoped it could be injected directly somewhere in the
decoding process with a much smaller cost.

For that reason, plus the fact that this API enhancement will not make it
into the next Debian Stable, I am hesitating to use it. Especially for
floats, where vector_fmul_scalar+vector_clipf can do the trick really in two
lines, possibly faster. I shall have to profile a bit.

But the feature will be useful for many other applications, so thanks.

> One motivation is because in integrated applications which have a
> volume control it would make sense to combine it with the volume
> control instead of always doing two layers of scaling.  Thats not of a
> reason to not also have a CTL for it, however.

This is a perfectly valid concern. I believe, if I was the one to design the
API, I would have enabled internal handling of the gain by default, so that
stupid applications would have a good output without bothering. But there is
no point in dwelling on what is already done.

> I suspected it might be easier to add a CTL for it and discussed it in
> the past as it could also have some minor precision benefits for the
> fixed point decoder and the integer interface, but doing so missed the
> last code freeze for the standardization process (there was a span of
> time where we couldn't change any implementation of Opus in any way in
> order to avoid "but it's not done yet!" FUD by people trying to
> disrupt the standards process). But, ultimately, the real answer is
> simply because no one cared to ask for anything else: Since no one
> implementing Opus support had asked it hadn't come back up again.

Thanks for the explanations.

I will rework my patches to take everything into consideration.

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/20120711/080d8d14/attachment.asc>