From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 989C647520 for ; Sun, 10 Sep 2023 22:56:05 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 4109968C923; Mon, 11 Sep 2023 01:56:02 +0300 (EEST) Received: from mail-ot1-f42.google.com (mail-ot1-f42.google.com [209.85.210.42]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6858D68C6D8 for ; Mon, 11 Sep 2023 01:55:56 +0300 (EEST) Received: by mail-ot1-f42.google.com with SMTP id 46e09a7af769-6bdacc5ed66so2078545a34.1 for ; Sun, 10 Sep 2023 15:55:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694386553; x=1694991353; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=JxxgiHUowDVvtWvVLV2tw/x0vBE+JvaDZDaLzF7vVNM=; b=A96WRSZ0po0sDXK2YyurDRMqaldY6oEvsrIahs3i3k8YEF+pLrgIHNn0cvXDWXfGQC Pe0xbcCBwyQVZ2r71Gd3vZH1Mu9+545SIvxUbyMGOMW83ZZA2URK1plJ7lg2s527dhE5 kl5tGw3V5h/ymy5Dn9XhmdpEZTmHIQi577DyTECa7w+hS9kMvkcnSJVdFP2kqCQXBcY+ 2xKwDUpa11j45/juaSI4ev7Ky2lxIheMgbrsJj6gRZz+HHvmgPxoKySDwSK9vo/FQead Kh9pllKClhEj35p4NnVeRCWJq8U0puP25JGrBR+uZvGo6nV72v6LNrdLUL+Gty0SvWQv 9qUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694386553; x=1694991353; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=JxxgiHUowDVvtWvVLV2tw/x0vBE+JvaDZDaLzF7vVNM=; b=gvLNbeUxvmurt8h6Ybgel4h7kfATKL4rLwWBKCQlsqj/67SYrY6Mf+PYwbqfmDOFUp i8jXvl8YNTlK0tMVJu6aWBo1CVYMKLlG87A5eecmzlkjx8XxF4bTJWbH1MLyuqFwdRvH DKhHqbxT9jkwZyMuI2kn4ElK+TxsUPWHRQzDNLaETQ65OV4zFXTVLsTppGrHUFARNRGg RDFX8Di4GU1y6DpmkrWSbKjOuyys5czuWm0313fcv7tjdUvGlPJbRm1MLpfiw1jr+bmH opLkwlj0JW3/mRBV4z3lcmcdQY4r3pTkVNtAe0fTVI0q4BZ+jypHLiPozRLRbF163BJS IfiQ== X-Gm-Message-State: AOJu0YzcO6eJzHBqibezyAAGspH9jiG8eF021bR+YZhmz/xuNINVAucT 31FRYM4f94V7h7+gBKiAhKGPzK8WgGk= X-Google-Smtp-Source: AGHT+IHoxJ7w2B7sRih54MV4sFliM25UcmXPKYx70I/kbPJ9Dz4FxU1/Krmlb3wJLncag7vcIqpKnw== X-Received: by 2002:a05:6830:204a:b0:6bc:d5a5:c2f with SMTP id f10-20020a056830204a00b006bcd5a50c2fmr4166551otp.9.1694386553172; Sun, 10 Sep 2023 15:55:53 -0700 (PDT) Received: from [192.168.0.10] (host197.190-225-105.telecom.net.ar. [190.225.105.197]) by smtp.gmail.com with ESMTPSA id s24-20020a9d7598000000b006b9e81d94a7sm2628328otk.70.2023.09.10.15.55.51 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 10 Sep 2023 15:55:52 -0700 (PDT) Message-ID: <9b63cd1e-414e-4614-f2a0-bc29a5c427ea@gmail.com> Date: Sun, 10 Sep 2023 19:56:01 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 To: ffmpeg-devel@ffmpeg.org References: <20230816111131.494-1-d.kozinski@samsung.com> Content-Language: en-US From: James Almer In-Reply-To: <20230816111131.494-1-d.kozinski@samsung.com> Subject: Re: [FFmpeg-devel] [PATCH v27 2/2] avcodec/evc_decoder: Provided support for EVC decoder X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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".