Re: [FFmpeg-devel] [PATCH 2/3] avcodec/rv60: RealVideo 6.0 decoder

[Anton Khirnov <anton@khirnov.net>]

Quoting Peter Ross (2023-10-18 10:03:54)
> +static int rv60_decode_frame(AVCodecContext *avctx, AVFrame * frame,
> +                             int * got_frame, AVPacket * avpkt)
> +{
> +    RV60Context *s = avctx->priv_data;
> +    GetBitContext gb;
> +    int ret, header_size, width, height, ofs;
> +
> +    if (avpkt->size == 0) {
> +        if (s->last_frame[NEXT_PIC]->data[0] && !s->got_last_frame_output) {
> +            if ((ret = av_frame_ref(frame, s->last_frame[NEXT_PIC])) < 0)
> +                return ret;
> +            s->got_last_frame_output = 1;
> +            *got_frame = 1;
> +        }

I think you can simplify this into:
    if (s->last_frame[NEXT_PIC]->data[0]) {
        av_frame_move_ref(frame, s->last_frame[NEXT_PIC]);
        *got_frame = 1;
    }

> +        return 0;
> +    }
> +
> +    if (avpkt->size < 9)
> +        return AVERROR_INVALIDDATA;
> +
> +    header_size = avpkt->data[0] * 8 + 9;
> +    if (avpkt->size < header_size)
> +        return AVERROR_INVALIDDATA;
> +
> +    init_get_bits8(&gb, avpkt->data + header_size, avpkt->size - header_size);
> +
> +    if ((ret = read_frame_header(s, &gb, &width, &height)) < 0)
> +        return ret;
> +
> +    if (avctx->skip_frame >= AVDISCARD_NONREF && s->pict_type == AV_PICTURE_TYPE_B ||
> +        avctx->skip_frame >= AVDISCARD_NONKEY && s->pict_type != AV_PICTURE_TYPE_I ||
> +        avctx->skip_frame >= AVDISCARD_ALL)
> +        return avpkt->size;
> +
> +    if (s->pict_type != AV_PICTURE_TYPE_B)
> +        FFSWAP(AVFrame *, s->last_frame[NEXT_PIC], s->last_frame[LAST_PIC]);
> +
> +    if ((s->pict_type == AV_PICTURE_TYPE_P && !s->last_frame[LAST_PIC]->data[0]) ||
> +        (s->pict_type == AV_PICTURE_TYPE_B && (!s->last_frame[LAST_PIC]->data[0] || !s->last_frame[NEXT_PIC]->data[0]))) {
> +        av_log(s->avctx, AV_LOG_ERROR, "missing reference frame\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    av_frame_unref(s->last_frame[CUR_PIC]);
> +
> +    s->last_frame[CUR_PIC]->pict_type = s->pict_type;
> +    if (s->pict_type == AV_PICTURE_TYPE_I)
> +        s->last_frame[CUR_PIC]->flags |= AV_FRAME_FLAG_KEY;
> +
> +    if ((ret = update_dimensions_clear_info(s, width, height)) < 0)
> +        return ret;
> +
> +    if ((ret = ff_reget_buffer(avctx, s->last_frame[CUR_PIC], 0)) < 0)

You just unreffed the frame above, what is the point of using
reget_buffer()?

> +        return ret;
> +
> +    if ((ret = read_slice_sizes(s, &gb)) < 0)
> +        return ret;
> +
> +    ofs = get_bits_count(&gb) / 8;
> +
> +    for (int i = 0; i < s->cu_height; i++) {
> +        s->slice[i].data = avpkt->data + header_size + ofs;
> +        s->slice[i].data_size = FFMIN(s->slice[i].size, avpkt->size - header_size - ofs);
> +        ofs += s->slice[i].size;
> +    }
> +
> +    if (ffcodec(avctx->codec)->update_thread_context)
> +        ff_thread_finish_setup(avctx);
> +
> +    ret = ff_slice_thread_allocz_entries(s->avctx, s->cu_height);
> +    if (ret < 0)
> +        return ret;
> +
> +    s->avctx->execute2(s->avctx, decode_slice, s->last_frame[CUR_PIC], NULL, s->cu_height);
> +
> +    frame->pkt_dts = avpkt->dts;

The generic code should be doing this already.

> +
> +    ret = 0;
> +    if (s->pict_type == AV_PICTURE_TYPE_B)
> +        ret = av_frame_ref(frame, s->last_frame[CUR_PIC]);

You could change this into av_frame_move_ref() and drop the unref below.

> +    else if (s->last_frame[LAST_PIC]->data[0])
> +        ret = av_frame_ref(frame, s->last_frame[LAST_PIC]);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (s->last_frame[LAST_PIC]->data[0])
> +        *got_frame = 1;
> +
> +    if (s->pict_type != AV_PICTURE_TYPE_B)
> +        FFSWAP(AVFrame *, s->last_frame[CUR_PIC], s->last_frame[NEXT_PIC]);
> +    else
> +        av_frame_unref(s->last_frame[CUR_PIC]);
> +
> +    if (s->pict_type != AV_PICTURE_TYPE_B) {
> +        s->ref_pts[0] = s->ref_pts[1];
> +        s->ref_pts[1] = avpkt->pts;
> +
> +        s->ref_ts[0] = s->ref_ts[1];
> +        s->ref_ts[1] = s->ts;
> +
> +        if (s->ref_pts[1] > s->ref_pts[0] && s->ref_ts[1] > s->ref_ts[0])
> +            s->ts_scale = (s->ref_pts[1] - s->ref_pts[0]) / (s->ref_ts[1] - s->ref_ts[0]);
> +    } else {
> +        frame->pts = s->ref_pts[0] + (s->ts - s->ref_ts[0]) * s->ts_scale;

This looks immensely evil. Isn't ff_get_buffer() already setting the
timestamps correctly?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".