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

["Dawid Kozinski/Multimedia \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics" <d.kozinski@samsung.com>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James
> Almer
> Sent: poniedziałek, 11 września 2023 00:56
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v27 2/2] avcodec/evc_decoder: Provided
> support for EVC decoder
> 
> 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?
> 

No, this doesn't mean that there can be more than one frame after a call to
xevd_decode() with one AU. One AU can only contain a single frame.
Additionally, it may contain NAL units of other types, such as SPS or PPS.

Each AU is broken down into into NAL units. If a NAL unit contains a frame,
stat.fnum has a value other than -1. If the NAL unit containing the frame
has stat.fnum set, it can be interpreted as an index for the decoded frame.

> > +
> > +                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.
> 
That is not like this. 
pkt_au (an AVPacket containing AU) is used as input for the decoder while
the xevd_decode() function is called, while pkt_au_imgb is got from the
decoder while the xevd_pull() function is called. 
The sequence of NAL units containing frame data that we put into the decoder
may not match the order of frames exiting the decoder (decoding sequence vs
presentation sequnce). Therefore, the concern about the av_packet_free()
call resulting in a use-after-free issue may not be valid, as these packets
do not necessarily point to the same memory.

> > +
> > +                            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.
> 

During the decoding of an EVC stream, the libxevd_receive_frame decoder
function is called (for the sake of precision, this is just the decoder
wrapper, not the decoder itself as the actual decoding is delegated to the
libxevd library). Inside this function, the ff_decode_get_packet() function
is called, and it is in charge of providing data to the decoder.

In the case of the EVC stream, the ff_decode_get_packet() function returns a
pointer to an AU (which is essentially a set of NAL units that are
consecutive in decoding order and contains exactly one encoded image) that
is provided in the data field of the AVPacket structure.

Then, in a while loop, the AU is decomposed into NAL units, which are
sequentially passed to the xevd_decode() function. Along with the NAL unit,
the xevd_decode() function also receives a pointer to the AVPacket structure
containing the AU to which the NAL unit belongs. Each AU contains only one
NAL unit containing an image.

If the decoder is able to provide a decoded frame, the xevd_decode()
function returns 0 and sets the fnum field of the XEVD_STAT structure to a
value greater than or equal to 0. fnum values can be considered as indices
for subsequent decoded frames.

Therefore, since there can be only one NAL unit containing encoded frame
data present in an AU, the xevd_decode() function will return only once fnum
which will be greater than or equal to 0. As a result, the
ff_decode_frame_props_from_pkt() function is called only once inside
libxevd_receive_frame, after the NAL unit containing frame data is extracted
from the AU.

> > +                        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://protect2.fireeye.com/v1/url?k=a3c34be9-c2bea395-a3c2c0a6-
> 74fe485cc33c-700162d83aeb15df&q=1&e=5bb81d7a-0d7d-43d7-ad83-
> 76da98475fdc&u=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmp
> eg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".


_______________________________________________
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".