[Ffmpeg-devel] truncated audio output

[Justin Ruggles]

Michael Niedermayer wrote:
> Hi
> 
> On Mon, Jun 19, 2006 at 12:11:26AM -0400, Justin Ruggles wrote:
> 
>>After re-reading my post, I figured that my idea might be better
>>explained by a patch.  Here is what it would look like if I modified the
>>pcm encoder to use my suggestion instead of frame_size=1, which is what
>>is used currently.
> 
> 
> i agree that a CODEC_CAP_VAR_FRAME_SIZE needs to be added ...
> 
> 
> [...]
> 
>>Index: ffmpeg/ffmpeg.c
>>===================================================================
>>--- ffmpeg/ffmpeg.c	(.../svn://svn.mplayerhq.hu/ffmpeg/trunk)	(revision 5497)
>>+++ ffmpeg/ffmpeg.c	(.../ffmpeg)	(working copy)
>>@@ -526,7 +526,7 @@
>>     }
>> 
>>     /* now encode as many frames as possible */
>>-    if (enc->frame_size > 1) {
>>+
>>         /* output resampled raw samples */
>>         fifo_write(&ost->fifo, buftmp, size_out,
>>                    &ost->fifo.wptr);
>>@@ -551,48 +551,6 @@
>> 
>>             ost->sync_opts += enc->frame_size;
>>         }
>>-    } else {
>>-        AVPacket pkt;
> 
> 
> i think we shouldnt drop support for leaving the packet size equal to
> the input where its supported, that said i dont mind changing the default
> to a fixed size if theres some advantage

I guess keeping equal-size-to-input support wouldn't be bad, but I don't
think a frame_size of 1 should be used to signal it.  An encoder might
need to really write a frame with size 1.  frame_size=0 might work, but
seems like an ugly way to do it.

I like the idea making a distinction between a codec supporting a
smaller last frame or supporting any frame size you throw at it (e.g.
PCM).  I don't like the idea of using frame_size=0 though, unless there
is some other way of indicating how many samples are being passed to the
encoder.  Another avctx field could work, but seems unnecessary.  I am
in favor of adding 2 capabilities, CODEC_CAP_VAR_FRAME_SIZE, and
CODEC_CAP_SMALL_LAST_FRAME.  A codec which supports the former could
choose to encode any number of samples thrown at it or else keep an
internal sample buffer & write encoded frames as necessary.  The latter
would just need a check for smaller frame size & it would know it's the
last frame.

> 
> [...]
> 
>>@@ -1357,12 +1315,32 @@
>>                 if (ost->encoding_needed) {
>>                     for(;;) {
>>                         AVPacket pkt;
>>+                        int fifo_bytes;
>>                         av_init_packet(&pkt);
>>                         pkt.stream_index= ost->index;
>> 
>>                         switch(ost->st->codec->codec_type) {
>>                         case CODEC_TYPE_AUDIO:
>>-                            ret = avcodec_encode_audio(enc, bit_buffer, bit_buffer_size, NULL);
>>+                            fifo_bytes = fifo_size(&ost->fifo, NULL);
>>+                            ret = 0;
>>+                            /* encode any samples remaining in fifo */
>>+                            if(fifo_bytes > 0 && enc->codec->capabilities & CODEC_CAP_VAR_FRAME_SIZE) {
>>+                                int fs_tmp = enc->frame_size;
>>+                                enc->frame_size = fifo_bytes / (2 * enc->channels);
>>+                                if(enc->frame_size > fs_tmp) {
>>+                                    enc->frame_size = fs_tmp;
>>+                                }
> 
> 
> why is that check needed? if we had more data then frame_size shouldnt that
> have been encoded already? and what about non 16bit PCM?
> 
> [...]
> 

You're right.  It's not needed.  I didn't have that check there at
first, but was trying to debug a problem I encountered and forgot to
remove it.

Non-16bit PCM is converted to 16-bit before it gets here.  Right now
16-bit samples coming from the decoder is assumed all throughout the
code.  I guess it could be good to add a check for input sample type for
future use when other bit-depths are supported internally, but changing
it everywhere would be a daunting task.  Needed...but daunting.

-Justin