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 EBDD3486E3 for ; Thu, 14 Dec 2023 23:33:46 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 9B9B668D282; Fri, 15 Dec 2023 01:33:43 +0200 (EET) Received: from mail-oo1-f46.google.com (mail-oo1-f46.google.com [209.85.161.46]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3B75868CD8D for ; Fri, 15 Dec 2023 01:33:37 +0200 (EET) Received: by mail-oo1-f46.google.com with SMTP id 006d021491bc7-5908b15f43eso28849eaf.1 for ; Thu, 14 Dec 2023 15:33:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1702596814; x=1703201614; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=pQsbyt7qZwsRoVVC6vnsTk+MTcaDJp55Z08kXoA4RUI=; b=DGd2sdeJJbBWn229mV/Ab37uK8Peonan3wwSQ2yhxFfEG2MELlA6CcOFauBCEx14XF iuu7B/tC6oVQ7+VXQi8DcW9X29Y+wsJK39UqxKS/trCNrDILJvmff2isKtRPmDwNQ8Zi UgC9agNy67/mPPug0ejqpWlHqcedVYXbeiMoobqdMOJXo9Z6LGq+6AOxLRygyJ1pyZaC LfhmEnwiRd1CSOULBhrMhHGsV2SIFoBsil9IPpEnmCTCj2XmDuldfIyhlwHsZvOUweV8 YYDUb5iUqCS/YRjsQBGlSzlRKMuHZgB9ehXb2Rfv+HdMDmXcwDV+ExlHnoCk7RmhuXP/ aqPw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702596814; x=1703201614; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=pQsbyt7qZwsRoVVC6vnsTk+MTcaDJp55Z08kXoA4RUI=; b=V9KE7TBTA16z5JkSABDT/2VWhJ9gdeiF6jQ43sZGAmYYHoz9OThk5yZpagKy7EMoSS mIGXAvTT5aaR7f6hcS+xV8tKTXtlWV/PJ9vCI+9vffZewCRvKXW8aeUU99z+at8BGhp9 HbwKcSXX3GiI6vFO3f3QaJu+KePIi1yEBrMUqLP6AaWvwvK/gd1GlH0b6vcF6JaKKWAh ACRHmz/duEfBn6QDjAa1srmYlPPaV13x7nNbTb9T81y9i6kwb/VT8ikvJ+iTJrKrrBaU aY/Ydbko/A+iHra2fRVveGgE4d36ZGfpxyB34Eg2gWkB4HuAwEisVAlqQaME1jEx5Ttn hhdA== X-Gm-Message-State: AOJu0YzrIemCoGjFlMZs6J7b0I/yi5zk4Z9pQeCL6z53nQdDNh0jT4Vg rddZk3tISeQ29G7HhbhVGmSL5vyjUvY= X-Google-Smtp-Source: AGHT+IFn3hJXD6kYq/sL4tApJZOhMg4QIpOnBUPWtVJA2ekWWckXrM10txfen+BrIOlzw1SJOvdyTQ== X-Received: by 2002:a05:6358:7e06:b0:16b:971c:98bc with SMTP id o6-20020a0563587e0600b0016b971c98bcmr24403877rwm.2.1702596814355; Thu, 14 Dec 2023 15:33:34 -0800 (PST) Received: from [192.168.1.35] (c-68-56-149-176.hsd1.mi.comcast.net. [68.56.149.176]) by smtp.gmail.com with ESMTPSA id cw8-20020ad44dc8000000b0067f0c0a17e1sm799878qvb.63.2023.12.14.15.33.33 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 14 Dec 2023 15:33:34 -0800 (PST) Message-ID: <2a220627-10e4-4807-950b-03b08331c926@gmail.com> Date: Thu, 14 Dec 2023 18:33:33 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US-large To: FFmpeg development discussions and patches References: <20231208173106.165084-1-leo.izen@gmail.com> <170254251501.8914.17063422483267428315@lain.khirnov.net> From: Leo Izen In-Reply-To: <170254251501.8914.17063422483267428315@lain.khirnov.net> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libjxldec: emit proper PTS to decoded AVFrame 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 12/14/23 03:28, Anton Khirnov wrote: > Quoting Leo Izen (2023-12-08 18:31:06) >> If a sequence of JXL images is encapsulated in a container that has PTS >> information, we should use the PTS information from the container. At >> this time there is no container that does this, but if JPEG XL support >> is ever added to NUT, AVTransport, or some other container, this commit >> should allow the PTS information those containers provide to work as >> expected. >> >> Signed-off-by: Leo Izen >> --- >> libavcodec/libjxldec.c | 77 +++++++++++++++++++++++++++++++----------- >> 1 file changed, 57 insertions(+), 20 deletions(-) >> >> diff --git a/libavcodec/libjxldec.c b/libavcodec/libjxldec.c >> index 002740d9c1..494060ac8c 100644 >> --- a/libavcodec/libjxldec.c >> +++ b/libavcodec/libjxldec.c >> @@ -370,6 +370,7 @@ static int libjxl_receive_frame(AVCodecContext *avctx, AVFrame *frame) >> >> while (1) { >> size_t remaining; >> + JxlFrameHeader header; >> >> if (!pkt->size) { >> av_packet_unref(pkt); >> @@ -428,13 +429,16 @@ static int libjxl_receive_frame(AVCodecContext *avctx, AVFrame *frame) >> } >> if ((ret = ff_set_dimensions(avctx, ctx->basic_info.xsize, ctx->basic_info.ysize)) < 0) >> return ret; >> + /* >> + * If animation is present, we use the timebase provided by >> + * the animated image itself. >> + * If the image is not animated, we use ctx->pts >> + * to refer to the frame number, not an actual >> + * PTS value, thus we may leave ctx->timebase unset. >> + */ >> if (ctx->basic_info.have_animation) >> ctx->timebase = av_make_q(ctx->basic_info.animation.tps_denominator, >> ctx->basic_info.animation.tps_numerator); >> - else if (avctx->pkt_timebase.num) >> - ctx->timebase = avctx->pkt_timebase; >> - else >> - ctx->timebase = AV_TIME_BASE_Q; >> continue; >> case JXL_DEC_COLOR_ENCODING: >> av_log(avctx, AV_LOG_DEBUG, "COLOR_ENCODING event emitted\n"); >> @@ -462,23 +466,24 @@ static int libjxl_receive_frame(AVCodecContext *avctx, AVFrame *frame) >> #endif >> continue; >> case JXL_DEC_FRAME: >> + /* Frame here refers to the Frame bundle, not a decoded picture */ >> av_log(avctx, AV_LOG_DEBUG, "FRAME event emitted\n"); >> - if (!ctx->basic_info.have_animation || ctx->prev_is_last) { >> + if (ctx->prev_is_last) { >> + /* >> + * The last frame sent was tagged as "is_last" which >> + * means this is a new image file altogether. >> + */ >> ctx->frame->pict_type = AV_PICTURE_TYPE_I; >> ctx->frame->flags |= AV_FRAME_FLAG_KEY; >> } >> - if (ctx->basic_info.have_animation) { >> - JxlFrameHeader header; >> - if (JxlDecoderGetFrameHeader(ctx->decoder, &header) != JXL_DEC_SUCCESS) { >> - av_log(avctx, AV_LOG_ERROR, "Bad libjxl dec frame event\n"); >> - return AVERROR_EXTERNAL; >> - } >> - ctx->prev_is_last = header.is_last; >> - ctx->frame_duration = header.duration; >> - } else { >> - ctx->prev_is_last = 1; >> - ctx->frame_duration = 1; >> + if (JxlDecoderGetFrameHeader(ctx->decoder, &header) != JXL_DEC_SUCCESS) { >> + av_log(avctx, AV_LOG_ERROR, "Bad libjxl dec frame event\n"); >> + return AVERROR_EXTERNAL; >> } >> + ctx->prev_is_last = header.is_last; >> + /* zero duration for animation means the frame is not presented */ >> + if (ctx->basic_info.have_animation && header.duration) >> + ctx->frame_duration = header.duration; >> continue; >> case JXL_DEC_FULL_IMAGE: >> /* full image is one frame, even if animated */ >> @@ -490,12 +495,44 @@ static int libjxl_receive_frame(AVCodecContext *avctx, AVFrame *frame) >> /* ownership is transfered, and it is not ref-ed */ >> ctx->iccp = NULL; >> } >> - if (avctx->pkt_timebase.num) { >> - ctx->frame->pts = av_rescale_q(ctx->pts, ctx->timebase, avctx->pkt_timebase); >> - ctx->frame->duration = av_rescale_q(ctx->frame_duration, ctx->timebase, avctx->pkt_timebase); >> + if (ctx->basic_info.have_animation) { >> + if (avctx->pkt_timebase.num) { >> + /* >> + * ideally, the demuxer set avctx->pkt_timebase to equal the animation's timebase >> + * or something strictly finer. This is true about the jpegxl_anim demuxer. >> + */ >> + ctx->frame->pts = av_rescale_q(ctx->pts, ctx->timebase, avctx->pkt_timebase); >> + ctx->frame->duration = av_rescale_q(ctx->frame_duration, ctx->timebase, avctx->pkt_timebase); >> + } else { >> + /* >> + * If we don't know the container timebase, we have to set the frame->timebase, >> + * even if it is currently ignored by most users. We don't have permission >> + * to set avctx->pkt_timebase. >> + */ >> + ctx->frame->time_base = ctx->timebase; >> + ctx->frame->pts = ctx->pts; >> + ctx->frame->duration = ctx->frame_duration; >> + } >> + } else if (avctx->pkt_timebase.num) { >> + if (pkt->pts != AV_NOPTS_VALUE) { >> + /* The container has provided the PTS for us, so we don't need to count frames. */ >> + ctx->frame->pts = pkt->pts; >> + } else { >> + /* >> + * The demuxer has provided us with a timebase, but not with PTS information. >> + * We use 1/1 as a dummy timebase, for 1fps as a dummy framerate, and set the >> + * PTS based on frame count. >> + */ >> + const AVRational dummy = {.num = 1, .den = 1}; >> + ctx->frame->pts = av_rescale_q(ctx->pts, dummy, avctx->pkt_timebase); >> + } >> + ctx->frame->duration = pkt->duration; >> } else { >> + /* >> + * There is no timing information. Set the frame PTS to frame counter. >> + */ >> ctx->frame->pts = ctx->pts; >> - ctx->frame->duration = ctx->frame_duration; >> + ctx->frame->duration = 0; > > This logic seems shady to me. Which part, specifically? The animated logic, or the non-animated logic? > The decoder should mess with pts as little > as possible and whenever it can just copy the packet value to the frame. > Any codec-level timestamps should not be trusted. In the case of animated JXL, codec-level timestamps are all that's available because the only demuxer is jpegxl_anim, which doesn't packetize the individual frames. > > Now this does not work when a single packet decodes into multiple > frames, then you have to add increments of frame duration to the > original packet pts. But you should still preserve the original value as > the base - it might not start at 0. I see what you're saying, but in the case where one packet decodes into multiple frames in the non-animated stream, we don't have any way to properly differentiate the PTS of those frames. In the animated case, this makes sense to me though. > > Also, decoders are not allowed to set AVFrame.time_base. And you should > probably set AVCodecContext.framerate. Animated JXL uses frame-delay and has no inherent framerate. I'm not sure what we'd set it to. I can remove the setting of AVFrame->time_base though. - Leo Izen (Traneptora) _______________________________________________ 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".