[FFmpeg-devel] [PATCH 2/4] avcodec/encode: restructure the core encoding code

[James Almer]

On 3/11/2020 7:18 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-02-27 19:02:00)
>> This commit follows the same logic as 061a0c14bb, but for the encode API: The
>> new public encoding API will no longer be a wrapper around the old deprecated
>> one, and the internal API used by the encoders now consists of a single
>> receive_packet() callback that pulls frames as required.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>  libavcodec/avcodec.h  |  12 +-
>>  libavcodec/encode.c   | 268 ++++++++++++++++++++++++++++++++----------
>>  libavcodec/encode.h   |  39 ++++++
>>  libavcodec/internal.h |   7 +-
>>  libavcodec/utils.c    |  12 +-
>>  5 files changed, 268 insertions(+), 70 deletions(-)
>>  create mode 100644 libavcodec/encode.h
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> +static int encode_simple_internal(AVCodecContext *avctx, AVPacket *avpkt)
>> +{
>> +    AVCodecInternal   *avci = avctx->internal;
>> +    EncodeSimpleContext *es = &avci->es;
>> +    AVFrame          *frame = es->in_frame;
>> +    AVFrame   *padded_frame = NULL;
>> +    int got_packet;
>>      int ret;
>> -    *got_packet = 0;
>>  
>> -    av_packet_unref(avctx->internal->buffer_pkt);
>> -    avctx->internal->buffer_pkt_valid = 0;
>> +    if (!frame->buf[0] && !avci->draining) {
>> +        av_frame_unref(frame);
>> +        ret = ff_encode_get_frame(avctx, frame);
>> +        if (ret < 0 && ret != AVERROR_EOF)
>> +            return ret;
>> +    }
>>  
>> -    if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
>> -        ret = avcodec_encode_video2(avctx, avctx->internal->buffer_pkt,
>> -                                    frame, got_packet);
>> -    } else if (avctx->codec_type == AVMEDIA_TYPE_AUDIO) {
>> -        ret = avcodec_encode_audio2(avctx, avctx->internal->buffer_pkt,
>> -                                    frame, got_packet);
>> -    } else {
>> -        ret = AVERROR(EINVAL);
>> +    if (avci->draining_done)
>> +        return AVERROR_EOF;
> 
> nit: this check would be better at the top of the function, since it
> shouldn't interact with the block above
> 
>> +
>> +    if (!frame->buf[0]) {
>> +        if (!(avctx->codec->capabilities & AV_CODEC_CAP_DELAY ||
>> +              avctx->active_thread_type & FF_THREAD_FRAME))
>> +            return AVERROR_EOF;
>> +
>> +        // Flushing is signaled with a NULL frame
>> +        frame = NULL;
>> +    }
>> +
>> +    got_packet = 0;
>> +
>> +    if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) {
>> +        if ((avctx->flags & AV_CODEC_FLAG_PASS1) && avctx->stats_out)
>> +            avctx->stats_out[0] = '\0';
>> +
>> +        if (av_image_check_size2(avctx->width, avctx->height, avctx->max_pixels, AV_PIX_FMT_NONE, 0, avctx)) {
>> +            ret = AVERROR(EINVAL);
>> +            goto end;
>> +        }
>> +        if (frame) {
>> +            if (frame->format == AV_PIX_FMT_NONE)
>> +                av_log(avctx, AV_LOG_WARNING, "AVFrame.format is not set\n");
>> +            if (frame->width == 0 || frame->height == 0)
>> +                av_log(avctx, AV_LOG_WARNING, "AVFrame.width or height is not set\n");
>> +        }
>> +    } else if (frame && avctx->codec->type == AVMEDIA_TYPE_AUDIO) {
>> +        /* extract audio service type metadata */
>> +        AVFrameSideData *sd = av_frame_get_side_data(frame, AV_FRAME_DATA_AUDIO_SERVICE_TYPE);
>> +        if (sd && sd->size >= sizeof(enum AVAudioServiceType))
>> +            avctx->audio_service_type = *(enum AVAudioServiceType*)sd->data;
>> +
>> +        /* check for valid frame size */
>> +        if (avctx->codec->capabilities & AV_CODEC_CAP_SMALL_LAST_FRAME) {
>> +            if (frame->nb_samples > avctx->frame_size) {
>> +                av_log(avctx, AV_LOG_ERROR, "more samples than frame size (avcodec_encode_audio2)\n");
>> +                ret = AVERROR(EINVAL);
>> +                goto end;
>> +            }
>> +        } else if (!(avctx->codec->capabilities & AV_CODEC_CAP_VARIABLE_FRAME_SIZE)) {
>> +            /* if we already got an undersized frame, that must have been the last */
>> +            if (avctx->internal->last_audio_frame) {
>> +                av_log(avctx, AV_LOG_ERROR, "frame_size (%d) was not respected for a non-last frame (avcodec_encode_audio2)\n", avctx->frame_size);
>> +                ret = AVERROR(EINVAL);
>> +                goto end;
>> +            }
>> +
>> +            if (frame->nb_samples < avctx->frame_size) {
>> +                ret = pad_last_frame(avctx, &padded_frame, frame);
>> +                if (ret < 0)
>> +                    goto end;
>> +
>> +                av_frame_unref(frame);
>> +                frame = padded_frame;
>> +                avctx->internal->last_audio_frame = 1;
>> +            }
>> +
>> +            if (frame->nb_samples != avctx->frame_size) {
>> +                av_log(avctx, AV_LOG_ERROR, "nb_samples (%d) != frame_size (%d) (avcodec_encode_audio2)\n", frame->nb_samples, avctx->frame_size);
>> +                ret = AVERROR(EINVAL);
>> +                goto end;
>> +            }
>> +        }
>> +    }
> 
> Nothing about this block looks specific to the simple encoding API. Some
> of those should be in the avcodec_send_frame(), others could possibly be
> dropped.

I kept the current behavior as intact as possible. All of this is only
ever executed for AVCodec.encode2() encoders, and not for
AVCodec.receive_packet() ones. Do you suggest i should try to make them
apply for all encoders instead? And which to drop?
And most, if not all of this, can't really be tested since there are
barely any receive_packet() encoders.

Same is done in the decode API, fwiw.