Re: [FFmpeg-devel] [PATCH v27 2/2] avcodec/evc_decoder: Provided support for EVC decoder

[James Almer <jamrial@gmail.com>]

On 8/16/2023 8:11 AM, Dawid Kozinski wrote:
> +/**
> + * Initialize decoder
> + * Create a decoder instance and allocate all the needed resources
> + *
> + * @param avctx codec context
> + * @return 0 on success, negative error code on failure
> + */
> +static av_cold int libxevd_init(AVCodecContext *avctx)
> +{
> +    XevdContext *xectx = avctx->priv_data;
> +    XEVD_CDSC *cdsc = &(xectx->cdsc);
> +
> +    /* read configurations and set values for created descriptor (XEVD_CDSC) */
> +    get_conf(avctx, cdsc);
> +
> +    /* create decoder */
> +    xectx->id = xevd_create(&(xectx->cdsc), NULL);
> +    if (xectx->id == NULL) {
> +        av_log(avctx, AV_LOG_ERROR, "Cannot create XEVD encoder\n");
> +        return AVERROR_EXTERNAL;
> +    }
> +
> +    xectx->draining_mode = 0;
> +    xectx->pkt = av_packet_alloc();

Unchecked allocation.

> +
> +    return 0;
> +}
> +
> +/**
> +  * Decode frame with decoupled packet/frame dataflow
> +  *
> +  * @param avctx codec context
> +  * @param[out] frame decoded frame
> +  *
> +  * @return 0 on success, negative error code on failure
> +  */
> +static int libxevd_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> +{
> +    XevdContext *xectx = avctx->priv_data;
> +    AVPacket *pkt = xectx->pkt;
> +    XEVD_IMGB *imgb = NULL;
> +
> +    int xevd_ret = 0;
> +    int ret = 0;
> +
> +    if (!pkt)
> +        return AVERROR(ENOMEM);

This check should be in libxevd_init(), like i said above.

> +
> +    // obtain access unit (input data) - a set of NAL units that are consecutive in decoding order and containing exactly one encoded image
> +    ret = ff_decode_get_packet(avctx, pkt);

You're unconditionally fetching a new packet every time receive_frame() 
is called. Is it guaranteed that the previous packet was fully consumed 
and freed?

> +    if (ret < 0 && ret != AVERROR_EOF) {
> +        av_packet_unref(pkt);
> +
> +        return ret;
> +    } else if(ret == AVERROR_EOF && xectx->draining_mode == 0) { // End of stream situations. Enter draining mode
> +
> +        xectx->draining_mode = 1;
> +        av_packet_unref(pkt);
> +    }
> +
> +    if (pkt->size > 0) {
> +        int bs_read_pos = 0;
> +        XEVD_STAT stat;
> +        XEVD_BITB bitb;
> +        int nalu_size;
> +        AVPacket* pkt_au;
> +        imgb = NULL;
> +
> +        pkt_au = av_packet_clone(pkt);

Unchecked allocation.

> +        av_packet_unref(pkt);

You're unreferencing this packet here but then you check its fields below.

> +
> +        // get all nal units from AU
> +        while(pkt_au->size > (bs_read_pos + XEVD_NAL_UNIT_LENGTH_BYTE)) {
> +            memset(&stat, 0, sizeof(XEVD_STAT));
> +
> +            nalu_size = read_nal_unit_length(pkt_au->data + bs_read_pos, XEVD_NAL_UNIT_LENGTH_BYTE, avctx);
> +            if (nalu_size == 0) {
> +                av_log(avctx, AV_LOG_ERROR, "Invalid bitstream\n");
> +                av_packet_free(&pkt_au);
> +                ret = AVERROR_INVALIDDATA;
> +
> +                return ret;
> +            }
> +            bs_read_pos += XEVD_NAL_UNIT_LENGTH_BYTE;
> +
> +            bitb.addr = pkt_au->data + bs_read_pos;
> +            bitb.ssize = nalu_size;
> +            bitb.pdata[0] = pkt_au;
> +            bitb.ts[XEVD_TS_DTS] = pkt_au->dts;
> +
> +            /* main decoding block */
> +            xevd_ret = xevd_decode(xectx->id, &bitb, &stat);
> +            if (XEVD_FAILED(xevd_ret)) {
> +                av_log(avctx, AV_LOG_ERROR, "Failed to decode bitstream\n");
> +                av_packet_free(&pkt_au);
> +                ret = AVERROR_EXTERNAL;

You can just do return AVERROR_EXTERNAL;

> +
> +                return ret;
> +            }
> +
> +            bs_read_pos += nalu_size;
> +
> +            if (stat.nalu_type == XEVD_NUT_SPS) { // EVC stream parameters changed
> +                if ((ret = export_stream_params(xectx, avctx)) != 0) {
> +                    av_log(avctx, AV_LOG_ERROR, "Failed to export stream params\n");
> +                    av_packet_free(&pkt_au);
> +
> +                    return ret;
> +                }
> +            }
> +
> +            if (stat.read != nalu_size)
> +                av_log(avctx, AV_LOG_INFO, "Different reading of bitstream (in:%d,read:%d)\n,", nalu_size, stat.read);
> +
> +            // stat.fnum - has negative value if the decoded data is not frame
> +            if (stat.fnum >= 0) {

This means there can be more than one frame after a call to 
xevd_decode() with one AU, right? Shouldn't you call xevd_pull() in a 
loop before you call xevd_decode() again, or attempt to fetch another 
packet/AU?

> +
> +                xevd_ret = xevd_pull(xectx->id, &imgb); // The function returns a valid image only if the return code is XEVD_OK
> +
> +                if (XEVD_FAILED(xevd_ret)) {
> +                    av_log(avctx, AV_LOG_ERROR, "Failed to pull the decoded image (xevd error code: %d, frame#=%d)\n", xevd_ret, stat.fnum);
> +                    ret = AVERROR_EXTERNAL;
> +                    av_packet_free(&pkt_au);
> +
> +                    return ret;
> +                } else if (xevd_ret == XEVD_OK_FRM_DELAYED) {
> +                    if(bs_read_pos == pkt->size) {

This is the check i was talking about being done with an empty packet. 
pkt->size will always be 0.

> +                        return AVERROR(EAGAIN);
> +                    }
> +                } else { // XEVD_OK
> +                    if (!imgb) {
> +                        if(bs_read_pos == pkt->size) {
> +                            av_log(avctx, AV_LOG_ERROR, "Invalid decoded image data\n");
> +
> +                            av_packet_free(&pkt_au);
> +                            return  AVERROR(EAGAIN);
> +                        }
> +                    } else {
> +                        // got frame
> +                        AVPacket* pkt_au_imgb = (AVPacket*)imgb->pdata[0];
> +                        if(!pkt_au_imgb) {
> +                            av_log(avctx, AV_LOG_ERROR, "Invalid data needed to fill frame properties\n");
> +
> +                            ret = AVERROR_INVALIDDATA;
> +
> +                            av_packet_free(&pkt_au);
> +
> +                            imgb->release(imgb);
> +                            imgb = NULL;
> +
> +                            av_frame_unref(frame);
> +
> +                            return ret;
> +                        }
> +
> +                        ret = libxevd_image_copy(avctx, imgb, frame);
> +                        if(ret < 0) {
> +                            av_log(avctx, AV_LOG_ERROR, "Image copying error\n");
> +
> +                            av_packet_free(&pkt_au);
> +                            av_packet_free(&pkt_au_imgb);

pkt_au and pkt_au_imgb both point to the same memory. This second 
av_packet_free() call will end in a use after free.

> +
> +                            imgb->release(imgb);
> +                            imgb = NULL;
> +
> +                            av_frame_unref(frame);
> +
> +                            return ret;
> +                        }
> +
> +                        // use ff_decode_frame_props_from_pkt() to fill frame properties
> +                        ret = ff_decode_frame_props_from_pkt(avctx, frame, pkt_au_imgb);

You attached the packet to imgb in order to fetch its props here, but 
are you sure it makes sense as is? This entire loop always uses the same 
packet you fetched at the start of the function. pkt_au_imgb will be the 
last packet the decoder saw and thus ff_get_buffer() will have set the 
frame with the same props already.

Using ff_decode_frame_props_from_pkt() with a packet you attached to 
some encoder handled struct is for the cases where the last packet you 
fetched is not the one with the props you want to use to fill this frame.

> +                        if (ret < 0) {
> +                            av_log(avctx, AV_LOG_ERROR, "ff_decode_frame_props_from_pkt error\n");
> +
> +                            av_packet_free(&pkt_au);
> +                            av_packet_free(&pkt_au_imgb);
> +
> +                            imgb->release(imgb);
> +                            imgb = NULL;
> +
> +                            av_frame_unref(frame);
> +
> +                            return ret;
> +                        }
> +
> +                        frame->pkt_dts = imgb->ts[XEVD_TS_DTS];
> +                        frame->pts = imgb->ts[XEVD_TS_PTS];
> +
> +                        // xevd_pull uses pool of objects of type XEVD_IMGB.
> +                        // The pool size is equal MAX_PB_SIZE (26), so release object when it is no more needed
> +                        imgb->release(imgb);
> +                        imgb = NULL;
> +
> +                        if(bs_read_pos == pkt->size) {
> +                            av_packet_free(&pkt_au);
> +                            av_packet_free(&pkt_au_imgb);
> +
> +                            av_frame_unref(frame);
> +                            return 0;
> +                        }
> +                    }
> +                }
> +            }
> +        }
> +    } else { // decoder draining mode handling
> +
> +        xevd_ret = xevd_pull(xectx->id, &imgb);
> +
> +        if (xevd_ret == XEVD_ERR_UNEXPECTED) { // draining process completed
> +            av_log(avctx, AV_LOG_DEBUG, "Draining process completed\n");
> +
> +            return AVERROR_EOF;
> +        } else if (XEVD_FAILED(xevd_ret)) { // handle all other errors
> +            av_log(avctx, AV_LOG_ERROR, "Failed to pull the decoded image (xevd error code: %d)\n", xevd_ret);
> +
> +            return AVERROR_EXTERNAL;
> +        } else { // XEVD_OK
> +            AVPacket* pkt_au_imgb;
> +            if (!imgb) {
> +                av_log(avctx, AV_LOG_ERROR, "Invalid decoded image data\n");
> +
> +                return AVERROR_EXTERNAL;
> +            }
> +
> +            pkt_au_imgb = (AVPacket*)imgb->pdata[0];
> +            if(!pkt_au_imgb) {
> +                av_log(avctx, AV_LOG_ERROR, "Invalid data needed to fill frame properties\n");
> +                ret = AVERROR_INVALIDDATA;
> +
> +                imgb->release(imgb);
> +                imgb = NULL;
> +
> +                av_frame_unref(frame);
> +
> +                return ret;
> +            }
> +
> +            // got frame
> +            ret = libxevd_image_copy(avctx, imgb, frame);
> +            if(ret < 0) {
> +                av_packet_free(&pkt_au_imgb);
> +                av_frame_unref(frame);
> +
> +                imgb->release(imgb);
> +                imgb = NULL;
> +
> +                return ret;
> +            }
> +            // use ff_decode_frame_props_from_pkt() to fill frame properties
> +            ret = ff_decode_frame_props_from_pkt(avctx, frame, pkt_au_imgb);
> +            if (ret < 0) {
> +                av_log(avctx, AV_LOG_ERROR, "ff_decode_frame_props_from_pkt error\n");
> +
> +                av_packet_free(&pkt_au_imgb);
> +                av_frame_unref(frame);
> +
> +                imgb->release(imgb);
> +                imgb = NULL;
> +
> +                return ret;
> +            }
> +
> +            frame->pkt_dts = imgb->ts[XEVD_TS_DTS];
> +            frame->pts = imgb->ts[XEVD_TS_PTS];
> +
> +            av_packet_free(&pkt_au_imgb);
> +
> +            // xevd_pull uses pool of objects of type XEVD_IMGB.
> +            // The pool size is equal MAX_PB_SIZE (26), so release object when it is no more needed
> +            imgb->release(imgb);
> +            imgb = NULL;
> +
> +            return 0;
> +        }
> +    }
> +
> +    return ret;
> +}
_______________________________________________
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".