On Fri, 2 Dec 2011, Justin Ruggles wrote:
On 12/02/2011 04:32 AM, Martin Storsjö wrote:
On Thu, 1 Dec 2011, Justin Ruggles wrote:
On 12/01/2011 06:24 PM, Martin Storsjö wrote:
Earlier, bits per sample was defined as 8, which wasn't chosen
with any thought, only chosen since it didn't seem to change
anything compared to not defining it at all.
g722 encodes 2 samples into one byte codeword, therefore the
bits per sample is 4. By changing this, the generated timestamps
for streams encoded with g722 become correct.
While technically true, this causes problems with WAV. In WAV files, the
bits-per-sample field in the header is supposed to be 8 (or 7 or 6) not
4. So we need an override for g722 in the WAV header writing function in
lavf to write the correct value.
Yeah, I came to think of this detail right after posting the patch - I
guess I chose 8 since it was the default for the "number of bits per
codeword/sample pair", which the decoder uses - my patch also makes the
raw g722 decoder set bits_per_coded_sample to 4, which gives warnings in
the decoder, which I realized after posting it.
As for WAV files, I don't have any samples, it'd be good to know what the
6/7 bits per codeword samples look like, if the words are packed tightly,
or the extra bits just are to be ignored. (Currently the decoder doesn't
assume tightly packed codewords.)
Well, the only sample I have been able to find is
http://samples.libav.org/A-codecs/g722/msn-messenger-g722.wav
And it actually has bits-per-sample as 16. :/ I also could not find a
G.722 codec for Windows to see how WMP handles it. I've never seen a 6/7
bits sample either, other than the raw ITU-T testsuite.
So maybe until we actually come across such samples in a container to
see how it's handled, we could just assume 4 bits per sample and provide
a decoder option for code_size.
Hmm, that might make sense.
One one hand, the bits per codeword is something that should be passed
from demuxer to decoder in the cases where the container format actually
knows that (which we don't really have any good cases of anyway), and for
that, a codec private option doesn't really work.
But for now, an option might be good enough until we have real samples
that need the more detailed handling, to decouple this from the bits per
sample definition.
See Bug #54
Oh, didn't know about this issue.
Due to the way avcodec_encode_audio is defined for non frame
based codecs, calculating the number of input samples in the
encode function has to be changed once the bits per samples
definition is changed.
It isn't defined that way for non-frame-based codecs, it's defined that
way for PCM. The g722 encoder is just wrong. All other ADPCM encoders
set frame_size and use it accordingly.
Hmm, so are there any other non-PCM encoders that don't set a frame size?
avcodec.h says this behaviour is for PCM codecs only, while avcodec.c does
it for any codec that has a frame_size <= 1.
AFAIK, PCM and G.722 are the only encoders which do not set a frame_size
1. But I haven't checked them all yet.
My issue (that pts isn't generated properly) is fixed by setting any
frame_size in the encoder init function (and changing the encode_frame
function accordingly), but does that make sense for a codec that can use
any frame size? (For this codec, the user could set any other frame size
he wants after initializing the encoder.) And if we'd want to change this
to use a default frame size, what would be a suitable default?
If the user changes frame_size after init with almost any encoder (with
the exception of the last frame for CODEC_CAP_SMALL_LAST_FRAME) it will
generally break horribly. I'm actually debating about how to handle PCM
and other such codecs in the new encoding API that I'm working on. One
option would be a CODEC_CAP that indicates that the encoder will take
any number of samples from the user (via AVFrame.nb_samples) and encode
them. Another option would be to have them behave like all the others
and set some arbitrary frame_size.
Ok - a more straightforward handling of these things in the new API really
is a welcome addition :-)
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
