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

[Anton Khirnov]

Sorry for the delay, got a bit busy here and forgot to reply.
Quoting James Almer (2020-03-11 14:27:54)
> 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.

The callers don't know or care whether the encoder is using the
"simple/old" API or the "new" one. All encoders should behave
consistently, unrelated behaviour shouldn't randomly change just because
we coverted and encoder to the new API.

>From a quick look I'd say:
video:
    - stats_out reset
    - image size check
audio:
    - audio service type
    - frame size checks and padding
should be done for all encoders.

The warnings for unset frame parameters look obsolete, so we might want
to drop them completely or keep them in the avcodec_encode_video2(), so
they get dropped along with that function.

-- 
Anton Khirnov