[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 at samsung.com
Thu Mar 30 13:09:14 EEST 2023
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of James
> Almer
> Sent: środa, 29 marca 2023 14:17
> To: ffmpeg-devel at 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 at 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 at ffmpeg.org
> with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list