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 3799E46211 for ; Sun, 9 Jul 2023 12:11:23 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A1BDB68C5F5; Sun, 9 Jul 2023 15:11:20 +0300 (EEST) Received: from mail-oi1-f174.google.com (mail-oi1-f174.google.com [209.85.167.174]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 5D955680040 for ; Sun, 9 Jul 2023 15:11:14 +0300 (EEST) Received: by mail-oi1-f174.google.com with SMTP id 5614622812f47-3942c6584f0so2602845b6e.3 for ; Sun, 09 Jul 2023 05:11:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1688904672; x=1691496672; 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=7Vs+phB7iA81Ar/K0lQv0ts9fx7qiZb/YL5NaIpZG3w=; b=Ci4AdnHQTCExqpoxmbrr21Kkp9pCDw7vMeCARR/MLeAHmN6lf6fFcoVQPqNacj6ucD 9f7FcDJ53WVQw8I9AgWACpW5OC/vgHdurPTjf1ag/kbEw0SfaWQn5kGKQlDvZSfUfHVW Hy7ew6UJFrSeBbYWNiNaKoH3abRxG7R7fTxXi4OtPbwhXKJjdq8F/UD48gztul535kEN leZS53dsSh2Zi61tzK8uw03MaoNLDUI8/Rv4uIj24Pq6acKms3lP9tSsUIFvUXHjygNa tehHZfszHiYtjqHOnEr/e26thCYQl3CQK4aJ9TXHvTAzv4cx1wzRxU5FsLpEBmk2970k n0vg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688904672; x=1691496672; 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=7Vs+phB7iA81Ar/K0lQv0ts9fx7qiZb/YL5NaIpZG3w=; b=fs+Rv7v3eHPDDQXUDtY1gZYts1382Vrc80Rs1vgdKRV50oqBh0FE4qni2Y/bkgylUV KyMM/V+7T3KMKmtk8XJCEcd8NicHURaX4zOI/RG50aLBlio55rzPVMTOE6IBRGy2AJpT HkFrRC7evjNcNaO3jF9WZ6F/unLshIwuTe628VXk3Vm/4u5NZgrkHl4s8tb99bi18RF0 +ymWlYoKhq8RpmRQoaJAM/o378NXCJX33kAEmM8r5pU6L6cnlpY3n46C1zFiOQvUAOlm IOeBe5Vs+sGNufXQmgFSGTeS9ojySfxMsoUJH3sRn3N36OMGgHAY21hWL1YksJfZUN/o qDgg== X-Gm-Message-State: ABy/qLapT3En379uuSwBaLHKpt1OCUtnkGBGWMMpC/imoYA0HnKzN/xN Wdo9gQRAw0S79o/X+xhCUoG5dj5l684= X-Google-Smtp-Source: APBJJlH2o+f6uno9Y0zu128OHVuqf7KMYRqLBuSOV0n+DKHRkzeVdco5GdoTu2AE6rUsJLSjFyHWBw== X-Received: by 2002:a05:6808:202a:b0:3a3:d153:2723 with SMTP id q42-20020a056808202a00b003a3d1532723mr10547747oiw.29.1688904672407; Sun, 09 Jul 2023 05:11:12 -0700 (PDT) Received: from [192.168.0.12] (host197.190-225-105.telecom.net.ar. [190.225.105.197]) by smtp.gmail.com with ESMTPSA id d15-20020a4ad34f000000b005634e8c4bbdsm3039123oos.11.2023.07.09.05.11.11 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 09 Jul 2023 05:11:12 -0700 (PDT) Message-ID: Date: Sun, 9 Jul 2023 09:11:14 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 To: ffmpeg-devel@ffmpeg.org References: <20230708190038.24324-1-jamrial@gmail.com> <168889609177.542.15252833672314979069@lain.khirnov.net> Content-Language: en-US From: James Almer In-Reply-To: <168889609177.542.15252833672314979069@lain.khirnov.net> Subject: Re: [FFmpeg-devel] [PATCH 1/3] avcodec/decode: move processing discard samples to its own function 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 7/9/2023 6:48 AM, Anton Khirnov wrote: > Quoting James Almer (2023-07-08 21:00:36) >> -/* >> - * The core of the receive_frame_wrapper for the decoders implementing >> - * the simple API. Certain decoders might consume partial packets without >> - * returning any output, so this function needs to be called in a loop until it >> - * returns EAGAIN. >> - **/ >> -static inline int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame, int64_t *discarded_samples) >> +static int discard_samples(AVCodecContext *avctx, AVFrame *frame, int64_t *discarded_samples) >> { >> - AVCodecInternal *avci = avctx->internal; >> - AVPacket *const pkt = avci->in_pkt; >> - const FFCodec *const codec = ffcodec(avctx->codec); >> - int got_frame, actual_got_frame; >> - int ret; >> - >> - if (!pkt->data && !avci->draining) { >> - av_packet_unref(pkt); >> - ret = ff_decode_get_packet(avctx, pkt); >> - if (ret < 0 && ret != AVERROR_EOF) >> - return ret; >> - } >> - >> - // Some codecs (at least wma lossless) will crash when feeding drain packets >> - // after EOF was signaled. >> - if (avci->draining_done) >> - return AVERROR_EOF; >> - >> - if (!pkt->data && >> - !(avctx->codec->capabilities & AV_CODEC_CAP_DELAY || >> - avctx->active_thread_type & FF_THREAD_FRAME)) >> - return AVERROR_EOF; >> - >> - got_frame = 0; >> - >> - if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME) { >> - ret = ff_thread_decode_frame(avctx, frame, &got_frame, pkt); >> - } else { >> - ret = codec->cb.decode(avctx, frame, &got_frame, pkt); >> - >> - if (!(codec->caps_internal & FF_CODEC_CAP_SETS_PKT_DTS)) >> - frame->pkt_dts = pkt->dts; >> - if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) { >> -#if FF_API_FRAME_PKT >> -FF_DISABLE_DEPRECATION_WARNINGS >> - if(!avctx->has_b_frames) >> - frame->pkt_pos = pkt->pos; >> -FF_ENABLE_DEPRECATION_WARNINGS >> -#endif >> - //FIXME these should be under if(!avctx->has_b_frames) >> - /* get_buffer is supposed to set frame parameters */ >> - if (!(avctx->codec->capabilities & AV_CODEC_CAP_DR1)) { >> - if (!frame->sample_aspect_ratio.num) frame->sample_aspect_ratio = avctx->sample_aspect_ratio; >> - if (!frame->width) frame->width = avctx->width; >> - if (!frame->height) frame->height = avctx->height; >> - if (frame->format == AV_PIX_FMT_NONE) frame->format = avctx->pix_fmt; >> - } >> - } >> - } >> - emms_c(); >> - actual_got_frame = got_frame; >> + AVCodecInternal *avci = avctx->internal; >> + int ret = 0; >> >> if (avctx->codec->type == AVMEDIA_TYPE_VIDEO) { >> if (frame->flags & AV_FRAME_FLAG_DISCARD) >> - got_frame = 0; >> + ret = AVERROR(EAGAIN); > > It's quite strange an unexpected for a function called discard_samples() > to do anything with video. By leaving video processing in the caller you > also save a level of indentation. I could call it discard_samples_or_frame(), or discard_frame(). If i process the video in the caller, it will end up being duplicated in patch 2, which goes against the point of factoring this out. > >> } else if (avctx->codec->type == AVMEDIA_TYPE_AUDIO) { >> uint8_t *side; >> size_t side_size; >> @@ -359,16 +304,10 @@ FF_ENABLE_DEPRECATION_WARNINGS >> uint8_t skip_reason = 0; >> uint8_t discard_reason = 0; >> >> - if (ret >= 0 && got_frame) { >> if (frame->format == AV_SAMPLE_FMT_NONE) >> frame->format = avctx->sample_fmt; >> - if (!frame->ch_layout.nb_channels) { >> - int ret2 = av_channel_layout_copy(&frame->ch_layout, &avctx->ch_layout); >> - if (ret2 < 0) { >> - ret = ret2; >> - got_frame = 0; >> - } >> - } >> + if (!frame->ch_layout.nb_channels) >> + ret = av_channel_layout_copy(&frame->ch_layout, &avctx->ch_layout); >> #if FF_API_OLD_CHANNEL_LAYOUT >> FF_DISABLE_DEPRECATION_WARNINGS >> if (!frame->channel_layout) >> @@ -380,7 +319,6 @@ FF_ENABLE_DEPRECATION_WARNINGS >> #endif >> if (!frame->sample_rate) >> frame->sample_rate = avctx->sample_rate; >> - } > > So the rest of the code below was previously executed even when there is > no frame? The change seems correct, but not I wouldn't expect it in a > 'move' commit, so perhaps mention it in the commit message. It still is, at least the block immediately after this, exporting avci->skip_samples. I don't know how correct it is to do that, but i did not want to change behavior in this set for decode() API decoders. > >> >> side= av_packet_get_side_data(avci->last_pkt_props, AV_PKT_DATA_SKIP_SAMPLES, &side_size); >> if(side && side_size>=10) { >> @@ -393,21 +331,21 @@ FF_ENABLE_DEPRECATION_WARNINGS >> discard_reason = AV_RL8(side + 9); >> } >> >> - if ((frame->flags & AV_FRAME_FLAG_DISCARD) && got_frame && >> + if ((frame->flags & AV_FRAME_FLAG_DISCARD) && !ret && >> !(avctx->flags2 & AV_CODEC_FLAG2_SKIP_MANUAL)) { >> avci->skip_samples = FFMAX(0, avci->skip_samples - frame->nb_samples); >> - got_frame = 0; >> *discarded_samples += frame->nb_samples; >> + ret = AVERROR(EAGAIN); > > If I'm reading correctly, all the blocks below are conditioned on !ret, > so you might as well return here and drop those checks. Ok. _______________________________________________ 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".