[7/7] libavcodec: Apply parameter change side data when decoding audio

Message ID 1324038961-51634-7-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Dec. 16, 2011, 12:36 p.m.
Should this be handled generically in avcodec_decode_audio4 like
this, or only within the decode function for those codecs that
are ready to handle a change. The latter variant avoids issues
if there are codecs that screw up if e.g. sample_rate/channels
are changed on the fly, but means a call to the parameter change
function has to be added to all functions. Currently, this is
tested to work fine with nellymoser.

A third approach would be to add a codec flag for this, and only
apply the side data packet for those codecs.
---
 libavcodec/utils.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

Comments

Justin Ruggles Dec. 16, 2011, 2:05 p.m. | #1
On 12/16/2011 07:36 AM, Martin Storsjö wrote:

> Should this be handled generically in avcodec_decode_audio4 like
> this, or only within the decode function for those codecs that
> are ready to handle a change. The latter variant avoids issues
> if there are codecs that screw up if e.g. sample_rate/channels
> are changed on the fly, but means a call to the parameter change
> function has to be added to all functions. Currently, this is
> tested to work fine with nellymoser.
> 
> A third approach would be to add a codec flag for this, and only
> apply the side data packet for those codecs.


I like the idea of adding a codec capability. At least until we know
that all decoders will be able to handle such mid-stream parameter
changes without blowing up.

Thanks,
Justin
Kostya Shishkov Dec. 16, 2011, 2:06 p.m. | #2
On Fri, Dec 16, 2011 at 09:05:31AM -0500, Justin Ruggles wrote:
> On 12/16/2011 07:36 AM, Martin Storsjö wrote:
> 
> > Should this be handled generically in avcodec_decode_audio4 like
> > this, or only within the decode function for those codecs that
> > are ready to handle a change. The latter variant avoids issues
> > if there are codecs that screw up if e.g. sample_rate/channels
> > are changed on the fly, but means a call to the parameter change
> > function has to be added to all functions. Currently, this is
> > tested to work fine with nellymoser.
> > 
> > A third approach would be to add a codec flag for this, and only
> > apply the side data packet for those codecs.
> 
> 
> I like the idea of adding a codec capability. At least until we know
> that all decoders will be able to handle such mid-stream parameter
> changes without blowing up.

What's the difference from what we have now? Parameter changes can still
happen mid-stream and decoder still have to deal with it even without knowing.
The advantage of side data is that it can be ignored by unsuspecting decoder.
Martin Storsjö Dec. 16, 2011, 2:19 p.m. | #3
On Fri, 16 Dec 2011, Kostya Shishkov wrote:

> On Fri, Dec 16, 2011 at 09:05:31AM -0500, Justin Ruggles wrote:
>> On 12/16/2011 07:36 AM, Martin Storsjö wrote:
>>
>>> Should this be handled generically in avcodec_decode_audio4 like
>>> this, or only within the decode function for those codecs that
>>> are ready to handle a change. The latter variant avoids issues
>>> if there are codecs that screw up if e.g. sample_rate/channels
>>> are changed on the fly, but means a call to the parameter change
>>> function has to be added to all functions. Currently, this is
>>> tested to work fine with nellymoser.
>>>
>>> A third approach would be to add a codec flag for this, and only
>>> apply the side data packet for those codecs.
>>
>>
>> I like the idea of adding a codec capability. At least until we know
>> that all decoders will be able to handle such mid-stream parameter
>> changes without blowing up.
>
> What's the difference from what we have now? Parameter changes can still
> happen mid-stream and decoder still have to deal with it even without knowing.
> The advantage of side data is that it can be ignored by unsuspecting decoder.

Parameter changes can happen yeah, but in most cases, we don't alter e.g. 
width/height/sample_rate/channels without the decoder knowing (currently, 
we'd just pass the new payload data to them).

The advantage of side data is that it can be ignored, I agree about that, 
but with the suggested param change side data type, the decoder doesn't 
need to do anything about the side data, the avcodec_decode_audio4 
function can apply the changes without having to have code in each decoder 
for it - but then we might want to know that the decoder is capable of 
handling it. That's what the three new patches I just sent addresses.

// Martin

Patch

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 68fc525..841c5b3 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -40,6 +40,7 @@ 
 #include "thread.h"
 #include "audioconvert.h"
 #include "internal.h"
+#include "bytestream.h"
 #include <stdlib.h>
 #include <stdarg.h>
 #include <limits.h>
@@ -924,6 +925,43 @@  int attribute_align_arg avcodec_decode_audio3(AVCodecContext *avctx, int16_t *sa
 }
 #endif
 
+static void apply_param_change(AVCodecContext *avctx, AVPacket *avpkt)
+{
+    int size = 0;
+    const uint8_t *data = av_packet_get_side_data(avpkt,
+                              AV_PKT_DATA_PARAM_CHANGE, &size);
+    uint32_t flags;
+    if (!data || size < 4)
+        return;
+    flags = bytestream_get_le32(&data);
+    size -= 4;
+    if (size < 4) /* Required for any of the changes */
+        return;
+    if (flags & AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT) {
+        avctx->channels = bytestream_get_le32(&data);
+        size -= 4;
+    }
+    if (flags & AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_LAYOUT) {
+        if (size < 8)
+            return;
+        avctx->channel_layout = bytestream_get_le64(&data);
+        size -= 8;
+    }
+    if (size < 4)
+        return;
+    if (flags & AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE) {
+        avctx->sample_rate = bytestream_get_le32(&data);
+        size -= 4;
+    }
+    if (flags & AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS) {
+        if (size < 8)
+            return;
+        avctx->width  = bytestream_get_le32(&data);
+        avctx->height = bytestream_get_le32(&data);
+        size -= 8;
+    }
+}
+
 int attribute_align_arg avcodec_decode_audio4(AVCodecContext *avctx,
                                               AVFrame *frame,
                                               int *got_frame_ptr,
@@ -940,6 +978,8 @@  int attribute_align_arg avcodec_decode_audio4(AVCodecContext *avctx,
         return AVERROR(EINVAL);
     }
 
+    apply_param_change(avctx, avpkt);
+
     if ((avctx->codec->capabilities & CODEC_CAP_DELAY) || avpkt->size) {
         ret = avctx->codec->decode(avctx, frame, got_frame_ptr, avpkt);
         if (ret >= 0 && *got_frame_ptr) {