[FFmpeg-devel,8/8] decklink: Add support for compressed AC-3 output over SDI

Extend the decklink output to include support for compressed AC-3,
encapsulated using the SMPTE ST 377:2015 standard.

This functionality can be exercised by using the "copy" codec when
the input audio stream is AC-3.  For example:

./ffmpeg -i ~/foo.ts -codec:a copy -f decklink 'UltraStudio Mini Monitor'

Note that the default behavior continues to be to do PCM output,
which means without specifying the copy codec a stream containing
AC-3 will be decoded and downmixed to stereo audio before output.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavdevice/decklink_enc.cpp | 101 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 86 insertions(+), 15 deletions(-)

2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>:

> +    uint8_t *outbuf = NULL;

> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3)
> +        av_free(outbuf);

The "if()" should not be necessary, free() and av_free()
may be called with argument "NULL".

Carl Eugen

> On Dec 29, 2017, at 3:59 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
> 2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller@ltnglobal.com>:
> 
>> +    uint8_t *outbuf = NULL;
> 
>> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3)
>> +        av_free(outbuf);
> 
> The "if()" should not be necessary, free() and av_free()
> may be called with argument "NULL”.

We don’t want to call av_free() if outbuf points to the original pkt->data field (i.e. if we’re receiving PCM audio).  We only want to free the memory if we allocated new memory to hold the S337 packet.

The concern isn’t the notion of calling av_free() if outbuf is NULL.  It’s about calling av_free() if outbuf points to pkt->data.

In short, I believe the logic here is correct.

Devin

On 12/29/2017 10:12 AM, Devin Heitmueller wrote:
> Extend the decklink output to include support for compressed AC-3,
> encapsulated using the SMPTE ST 377:2015 standard.
> 
> This functionality can be exercised by using the "copy" codec when
> the input audio stream is AC-3.  For example:
> 
> ./ffmpeg -i ~/foo.ts -codec:a copy -f decklink 'UltraStudio Mini Monitor'
> 
> Note that the default behavior continues to be to do PCM output,
> which means without specifying the copy codec a stream containing
> AC-3 will be decoded and downmixed to stereo audio before output.
> 
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
>   libavdevice/decklink_enc.cpp | 101 ++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 86 insertions(+), 15 deletions(-)
> 
> diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
> index 59a4181e19..aca3c13861 100644
> --- a/libavdevice/decklink_enc.cpp
> +++ b/libavdevice/decklink_enc.cpp
> @@ -224,19 +224,32 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
>           av_log(avctx, AV_LOG_ERROR, "Only one audio stream is supported!\n");
>           return -1;
>       }
> -    if (c->sample_rate != 48000) {
> -        av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
> -               " Only 48kHz is supported.\n");
> -        return -1;
> -    }
> -    if (c->channels != 2 && c->channels != 8 && c->channels != 16) {
> -        av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
> -               " Only 2, 8 or 16 channels are supported.\n");
> +
> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
> +        /* Regardless of the number of channels in the codec, we're only
> +           using 2 SDI audio channels at 48000Hz */
> +        ctx->channels = 2;
> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_PCM_S16LE) {
> +        if (c->channels != 2 && c->channels != 8 && c->channels != 16) {
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
> +                   " Only 2, 8 or 16 channels are supported.\n");
> +            return -1;
> +        }
> +        if (c->sample_rate != bmdAudioSampleRate48kHz) {
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
> +                   " Only 48kHz is supported.\n");
> +            return -1;
> +        }
> +        ctx->channels = c->channels;
> +    } else {
> +        av_log(avctx, AV_LOG_ERROR, "Unsupported codec specified!"
> +               " Only PCM_S16LE and AC-3 are supported.\n");
>           return -1;
>       }
> +
>       if (ctx->dlo->EnableAudioOutput(bmdAudioSampleRate48kHz,
>                                       bmdAudioSampleType16bitInteger,
> -                                    c->channels,
> +                                    ctx->channels,
>                                       bmdAudioOutputStreamTimestamped) != S_OK) {
>           av_log(avctx, AV_LOG_ERROR, "Could not enable audio output!\n");
>           return -1;
> @@ -247,8 +260,7 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
>       }
>   
>       /* The device expects the sample rate to be fixed. */
> -    avpriv_set_pts_info(st, 64, 1, c->sample_rate);
> -    ctx->channels = c->channels;
> +    avpriv_set_pts_info(st, 64, 1, bmdAudioSampleRate48kHz);
>   
>       ctx->audio = 1;
>   
> @@ -536,25 +548,84 @@ static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
>       return 0;
>   }
>   
> +static int create_s337_payload(AVPacket *pkt, enum AVCodecID codec_id, uint8_t **outbuf, int *outsize)
> +{
> +    uint8_t *s337_payload;
> +    uint8_t *s337_payload_start;
> +    int i;
> +
> +    /* Encapsulate AC3 syncframe into SMPTE 337 packet */
> +    *outsize = (pkt->size + 4) * sizeof(uint16_t);
> +    s337_payload = (uint8_t *) av_mallocz(*outsize);
> +    if (s337_payload == NULL)
> +        return AVERROR(ENOMEM);
> +
> +    *outbuf = s337_payload;

Should not store s337_payload in outbuf until the end of the function 
and returning success.  If it fails prematurely (say with the call to 
AVERROR(EINVAL)), the caller may rightfully interpret this to mean that 
outbuf has not been filled in and let outbuf leak.  In the case of 
failure, this function should call av_free() on s337_payload.  Also 
technically true for outsize as well--best to only modify output 
parameters when success is guaranteed.

> +
> +    /* Construct SMPTE S337 Burst preamble */
> +    s337_payload[0] = 0x72; /* Sync Word 1 */
> +    s337_payload[1] = 0xf8; /* Sync Word 1 */
> +    s337_payload[2] = 0x1f; /* Sync Word 1 */
> +    s337_payload[3] = 0x4e; /* Sync Word 1 */
> +
> +    if (codec_id == AV_CODEC_ID_AC3) {
> +        s337_payload[4] = 0x01;
> +    } else {
> +        return AVERROR(EINVAL);
> +    }
> +
> +    s337_payload[5] = 0x00;
> +    uint16_t bitcount = pkt->size * 8;
> +    s337_payload[6] = bitcount & 0xff; /* Length code */
> +    s337_payload[7] = bitcount >> 8; /* Length code */
> +    s337_payload_start = &s337_payload[8];
> +    for (i = 0; i < pkt->size; i += 2) {
> +        s337_payload_start[0] = pkt->data[i+1];
> +        s337_payload_start[1] = pkt->data[i];
> +        s337_payload_start += 2;
> +    }
> +
> +    return 0;
> +}
> +
>   static int decklink_write_audio_packet(AVFormatContext *avctx, AVPacket *pkt)
>   {
>       struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
>       struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
> -    int sample_count = pkt->size / (ctx->channels << 1);
> +    AVStream *st = avctx->streams[pkt->stream_index];
> +    int sample_count;
>       buffercount_type buffered;
> +    uint8_t *outbuf = NULL;
> +    int ret = 0;
>   
>       ctx->dlo->GetBufferedAudioSampleFrameCount(&buffered);
>       if (pkt->pts > 1 && !buffered)
>           av_log(avctx, AV_LOG_WARNING, "There's no buffered audio."
>                  " Audio will misbehave!\n");
>   
> -    if (ctx->dlo->ScheduleAudioSamples(pkt->data, sample_count, pkt->pts,
> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
> +        /* Encapsulate AC3 syncframe into SMPTE 337 packet */
> +        ret = create_s337_payload(pkt, st->codecpar->codec_id,
> +                                  &outbuf, &sample_count);
> +        if (ret != 0)

Assuming that you make the change discussed above to 
create_s337_payload(), can change to return ret here in case of failure.

> +            goto done;
> +    } else {
> +        sample_count = pkt->size / (ctx->channels << 1);
> +        outbuf = pkt->data;
> +    }
> +
> +    if (ctx->dlo->ScheduleAudioSamples(outbuf, sample_count, pkt->pts,
>                                          bmdAudioSampleRate48kHz, NULL) != S_OK) {

Simple approach that eliminates the need for using goto and the done 
label:  store the return value of ScheduleAudioSamples() in a local 
variable.  Then, free outbuf if appropriate.  Then handle the 
ScheduleAudioSamples() failure case.  Eliminating the goto will also 
make the code easier to understand, and it will result in the memory 
being deallocated immediately after it is no longer needed.

>           av_log(avctx, AV_LOG_ERROR, "Could not schedule audio samples.\n");
> -        return AVERROR(EIO);
> +        ret = AVERROR(EIO);
> +        goto done;
>       }
>   
> -    return 0;
> +done:
> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3)
> +        av_free(outbuf);
> +
> +    return ret;
>   }
>   
>   extern "C" {
> 

Aaron Levinson

Hi Aaron,

>>  +static int create_s337_payload(AVPacket *pkt, enum AVCodecID codec_id, uint8_t **outbuf, int *outsize)
>> +{
>> +    uint8_t *s337_payload;
>> +    uint8_t *s337_payload_start;
>> +    int i;
>> +
>> +    /* Encapsulate AC3 syncframe into SMPTE 337 packet */
>> +    *outsize = (pkt->size + 4) * sizeof(uint16_t);
>> +    s337_payload = (uint8_t *) av_mallocz(*outsize);
>> +    if (s337_payload == NULL)
>> +        return AVERROR(ENOMEM);
>> +
>> +    *outbuf = s337_payload;
> 
> Should not store s337_payload in outbuf until the end of the function and returning success.  If it fails prematurely (say with the call to AVERROR(EINVAL)), the caller may rightfully interpret this to mean that outbuf has not been filled in and let outbuf leak.  In the case of failure, this function should call av_free() on s337_payload.  Also technically true for outsize as well--best to only modify output parameters when success is guaranteed.

I actually had it that way in the original version, but it got moved during some refactoring.  That said, agreed it makes sense for the function to take responsibility for freeing the memory and not setting the output until the end of the function.

>>  -    if (ctx->dlo->ScheduleAudioSamples(pkt->data, sample_count, pkt->pts,
>> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
>> +        /* Encapsulate AC3 syncframe into SMPTE 337 packet */
>> +        ret = create_s337_payload(pkt, st->codecpar->codec_id,
>> +                                  &outbuf, &sample_count);
>> +        if (ret != 0)
> 
> Assuming that you make the change discussed above to create_s337_payload(), can change to return ret here in case of failure.

Agreed

> 
>> +            goto done;
>> +    } else {
>> +        sample_count = pkt->size / (ctx->channels << 1);
>> +        outbuf = pkt->data;
>> +    }
>> +
>> +    if (ctx->dlo->ScheduleAudioSamples(outbuf, sample_count, pkt->pts,
>>                                         bmdAudioSampleRate48kHz, NULL) != S_OK) {
> 
> Simple approach that eliminates the need for using goto and the done label:  store the return value of ScheduleAudioSamples() in a local variable.  Then, free outbuf if appropriate.  Then handle the ScheduleAudioSamples() failure case.  Eliminating the goto will also make the code easier to understand, and it will result in the memory being deallocated immediately after it is no longer needed.

Sure, with the above change we’ve now only got one case where we have to free the memory, there’s no need for the goto.

Devin