Re: [FFmpeg-devel] [PATCH v18 06/10] 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: środa, 29 marca 2023 14:17
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v18 06/10] avcodec/evc_decoder:
Provided
> support for EVC decoder
> 
> On 3/28/2023 10:50 AM, Dawid Kozinski wrote:
> > +static int libxevd_receive_frame(AVCodecContext *avctx, AVFrame
> > +*frame) {
> > +    XevdContext *xectx = NULL;
> > +    AVPacket *pkt = NULL;
> > +    XEVD_IMGB *imgb = NULL;
> > +
> > +    int xevd_ret = 0;
> > +    int ret = 0;
> > +
> > +    xectx = avctx->priv_data;
> > +
> > +    pkt = av_packet_alloc();
> > +    if (!pkt)
> > +        return AVERROR(ENOMEM);
> 
> This is very inefficient. Just keep an AVPacket in the XevdContext,
allocating it on
> init(), unrefing it after being used in this function, and freeing it on
close().
> 
> > +
> > +    // obtain input data
> > +    ret = ff_decode_get_packet(avctx, pkt);
> > +    if (ret < 0 && ret != AVERROR_EOF) {
> > +        av_packet_free(&pkt);
> > +        return ret;
> > +    } else if(ret == AVERROR_EOF && xectx->draining_mode == 0) { //
> > + End of stream situations. Enter draining mode
> > +
> > +        frame = NULL;
> > +        xectx->draining_mode = 1;
> > +
> > +        av_packet_free(&pkt);
> > +
> > +        return AVERROR_EOF;
> 
> By returning EOF here you're effectively ending the decoding process. No
> draining is taking place. This function is not going to be called again
for the
> xectx->draining_mode == 1 to take effect.
> 
> > +    }
> > +
> > +    if (pkt->size > 0) {
> > +        int bs_read_pos = 0;
> > +        XEVD_STAT stat;
> > +        XEVD_BITB bitb;
> > +        int nalu_size;
> > +
> > +        imgb = NULL;
> > +
> > +        while(pkt->size > (bs_read_pos + XEVD_NAL_UNIT_LENGTH_BYTE)) {
> > +            memset(&stat, 0, sizeof(XEVD_STAT));
> > +
> > +            nalu_size = read_nal_unit_length(pkt->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);
> > +                ret = AVERROR_INVALIDDATA;
> > +                return ret;
> > +            }
> > +            bs_read_pos += XEVD_NAL_UNIT_LENGTH_BYTE;
> > +
> > +            bitb.addr = pkt->data + bs_read_pos;
> > +            bitb.ssize = nalu_size;
> > +
> > +            /* 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);
> > +                ret = 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);
> > +                    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);
> > +            if (stat.fnum >= 0) {
> > +                // already has a decoded image
> > +                if (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;
> 
> Aren't you dropping an image you should be propagating instead with this?
> 
> > +                }
> > +                xevd_ret = xevd_pull(xectx->id, &imgb);
> > +                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);
> > +
> > +                    return ret;
> > +                }  else if (xevd_ret == XEVD_OK_FRM_DELAYED) {
> > +                    if (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;
> 
> Can an image be returned when xevd_ret == XEVD_OK_FRM_DELAYED? If so,
> shouldn't you propagate it?
> 
> > +                    }
> > +                    av_packet_free(&pkt);
> > +
> > +                    return AVERROR(EAGAIN);
> > +                }
> > +                if (imgb) { // got frame
> > +                    int ret = libxevd_image_copy(avctx, imgb, frame);
> > +                    if(ret < 0) {
> > +                        av_log(avctx, AV_LOG_ERROR, "Image copying
error\n");
> > +                        if (imgb) {
> > +                            imgb->release(imgb);
> > +                            imgb = NULL;
> > +                        }
> > +                        av_packet_free(&pkt);
> > +
> > +                        return ret;
> > +                    }
> > +
> > +                    // use ff_decode_frame_props() to fill frame
properties
> > +                    ret = ff_decode_frame_props(avctx, frame);
> > +                    if (ret < 0) {
> > +                        if (imgb) {
> > +                            imgb->release(imgb);
> > +                            imgb = NULL;
> > +                        }
> > +                        av_packet_free(&pkt);
> > +                        av_frame_unref(frame);
> > +
> > +                        return ret;
> > +                    }
> > +                    // match timestamps and packet size
> > +                    ret = ff_decode_frame_props_from_pkt(avctx, frame,
pkt);
> > +                    pkt->opaque = NULL;
> 
> You copy-pasted this from libdav1d.c without knowing what it's doing.
> You don't need to clear opaque, and you don't need to call
> ff_decode_frame_props_from_pkt() with the current packet when
> ff_decode_frame_props() is already doing it for you. The former function
is only
> needed when you need props from a packet other than the last one fed to
the
> decoder.
> 
> > +                    if (ret < 0) {
> > +                        if (imgb) {
> > +                            imgb->release(imgb);
> > +                            imgb = NULL;
> > +                        }
> > +                        av_packet_free(&pkt);
> > +                        av_frame_unref(frame);
> > +
> > +                        return ret;
> > +                    }
> > +
> > +                    frame->pkt_dts = pkt->dts;
> > +
> > +                    // 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;
> > +                } else
> > +                    return  AVERROR(EAGAIN);
> > +            }
> > +        }
> > +    } else { // decoder draining mode handling
> > +        xevd_ret = xevd_pull(xectx->id, &imgb);
> > +        if (xevd_ret == XEVD_ERR_UNEXPECTED)   // draining process
completed
> > +            return AVERROR_EOF;
> > +        else if (XEVD_FAILED(xevd_ret)) {
> > +            av_log(avctx, AV_LOG_ERROR, "Failed to pull the decoded
> > + image (xevd error code: %d)\n", xevd_ret);
> > +
> > +            if (imgb) {
> > +                imgb->release(imgb);
> > +                imgb = NULL;
> > +            }
> > +
> > +            av_packet_free(&pkt);
> > +
> > +            return AVERROR_EXTERNAL;
> > +        }
> > +        if (imgb) { // got frame
> > +            int ret = libxevd_image_copy(avctx, imgb, frame);
> > +            if(ret < 0) {
> > +                if (imgb) {
> > +                    imgb->release(imgb);
> > +                    imgb = NULL;
> > +                }
> > +                av_packet_free(&pkt);
> > +            }
> > +
> > +            frame->pkt_dts = pkt->dts;
> > +
> > +            // 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 AVERROR_EOF;
> > +    }
> > +    return ret;
> > +}
> ___


I'm sorry for not replying earlier but I've been experiencing an internet
connection issue since yesterday. I had an outage and no network connection.

I will make all the necessary changes in the code and upload new patches as
fast as possible.

____________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://protect2.fireeye.com/v1/url?k=a9b4cba4-c83fde97-a9b540eb-
> 000babff9bb7-057645cded2a2511&q=1&e=a107ddf9-a9a2-4d0b-ac05-
> 20a74e0f8ff9&u=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmpe
> g-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".