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

[Devin Heitmueller]

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