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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
