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 1360342BB2 for ; Tue, 30 Aug 2022 15:27:14 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B8CDB68B8C9; Tue, 30 Aug 2022 18:27:10 +0300 (EEST) Received: from mail-oa1-f43.google.com (mail-oa1-f43.google.com [209.85.160.43]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C327B68B3B2 for ; Tue, 30 Aug 2022 18:27:04 +0300 (EEST) Received: by mail-oa1-f43.google.com with SMTP id 586e51a60fabf-11eb44f520dso13260630fac.10 for ; Tue, 30 Aug 2022 08:27:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=DgLirpMVpyLYGxF94BZxo5gcgOswTjQrKNWtAW6czxA=; b=aJic3sySS6f0LMNkAg3AaHo8dIJs4CA0WB2zW0M0bfpayDulvEzzAU5psappjIjg0C 1hfTDYYIPbw7woCj2B0u+WXir39NqNYmqtusOjA72c+2/QL9ZrGXppV/1vWXbotVSnIX EzgHszTl2inUh335N+sT3JAzcXyLnnV+cTtjSaRj/JlrUsK0UtvnS4pqcnbaCm6R8X0/ +Njf49NP6lHisCAESUyPPwgyA7CHwXgzCJLzWpMC3ri/cbEbapkJzs6MPumfxZa754h3 WbVdlWPzFlSTr8iMNGVCHTMwST8litPIhn6v/ymABv1aYH44FLTeJrW7FK/edvRePOuo y7hA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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; bh=DgLirpMVpyLYGxF94BZxo5gcgOswTjQrKNWtAW6czxA=; b=OBheBNkH2mwfsGBi5ZqHmNZ7M/G8qpCDengtyp4r9mJ6NJnn4Q6zejshGfEO+g6Ked wxeF3MDiD/W612aLCt3dscPssuFleHDwspOuQyLJ8jIU7zrXGCNiAjfggFjF68TCfc2F nf8yljnGo79CUdC7TFhoqZb4DJNIxQrLzde4pqT4fIaTr5jbiq5E/XvjMARU+r7yWCzi QY8r3R09lfa7J6agHZh13zvnVoBq7OkGIXNAoPWK3Hsv7rBB/7PsiJ1abmh9ssbqMF3Z iF1JgRJvbFd7xV4nloqj9EIvpx4Zi+5/GggsimmfXXIQmn8sDDtvFfzMunZvAKdzBW+K DHzw== X-Gm-Message-State: ACgBeo0XA6XgkBmqKQCWdikuALBPT8Su5n24IU77PbFI7eL+nOBA109b bCnh0MLi9PFoDcq0MJqYaDLx9B3dyLI= X-Google-Smtp-Source: AA6agR4EQuEdxCsjtkVli8WoAxppC+LGHvwi7j6j4KAxauhkJzEUZMjd2pEixZnuz1Y6mF2fCAT8iw== X-Received: by 2002:a05:6808:201c:b0:343:b55:ae85 with SMTP id q28-20020a056808201c00b003430b55ae85mr9537189oiw.185.1661873221914; Tue, 30 Aug 2022 08:27:01 -0700 (PDT) Received: from [192.168.0.13] ([191.97.187.183]) by smtp.gmail.com with ESMTPSA id p37-20020a056870832500b0011e45afa4cbsm7939281oae.54.2022.08.30.08.27.00 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 08:27:01 -0700 (PDT) Message-ID: <60379b36-5d8a-d6b7-8e42-c7529095d9db@gmail.com> Date: Tue, 30 Aug 2022 12:26:59 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20220830014653.2477-1-jamrial@gmail.com> From: James Almer In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] avcodec: add a bsf to reorder DTS into PTS 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/30/2022 11:30 AM, Andreas Rheinhardt wrote: > James Almer: >> Starting with an h264 implementation. Can be extended to support other codecs. >> >> Addresses ticket #502. >> >> Signed-off-by: James Almer >> --- >> configure | 1 + >> libavcodec/Makefile | 1 + >> libavcodec/bitstream_filters.c | 1 + >> libavcodec/dts2pts_bsf.c | 477 +++++++++++++++++++++++++++++++++ >> 4 files changed, 480 insertions(+) >> create mode 100644 libavcodec/dts2pts_bsf.c >> >> diff --git a/configure b/configure >> index 932ea5b553..91ee5eb303 100755 >> --- a/configure >> +++ b/configure >> @@ -3275,6 +3275,7 @@ aac_adtstoasc_bsf_select="adts_header mpeg4audio" >> av1_frame_merge_bsf_select="cbs_av1" >> av1_frame_split_bsf_select="cbs_av1" >> av1_metadata_bsf_select="cbs_av1" >> +dts2pts_bsf_select="cbs_h264 h264parse" >> eac3_core_bsf_select="ac3_parser" >> filter_units_bsf_select="cbs" >> h264_metadata_bsf_deps="const_nan" >> diff --git a/libavcodec/Makefile b/libavcodec/Makefile >> index cb80f73d99..858e110b79 100644 >> --- a/libavcodec/Makefile >> +++ b/libavcodec/Makefile >> @@ -1176,6 +1176,7 @@ OBJS-$(CONFIG_AV1_FRAME_SPLIT_BSF) += av1_frame_split_bsf.o >> OBJS-$(CONFIG_CHOMP_BSF) += chomp_bsf.o >> OBJS-$(CONFIG_DUMP_EXTRADATA_BSF) += dump_extradata_bsf.o >> OBJS-$(CONFIG_DCA_CORE_BSF) += dca_core_bsf.o >> +OBJS-$(CONFIG_DTS2PTS_BSF) += dts2pts_bsf.o >> OBJS-$(CONFIG_DV_ERROR_MARKER_BSF) += dv_error_marker_bsf.o >> OBJS-$(CONFIG_EAC3_CORE_BSF) += eac3_core_bsf.o >> OBJS-$(CONFIG_EXTRACT_EXTRADATA_BSF) += extract_extradata_bsf.o \ >> diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c >> index 444423ae93..a3bebefe5f 100644 >> --- a/libavcodec/bitstream_filters.c >> +++ b/libavcodec/bitstream_filters.c >> @@ -31,6 +31,7 @@ extern const FFBitStreamFilter ff_av1_metadata_bsf; >> extern const FFBitStreamFilter ff_chomp_bsf; >> extern const FFBitStreamFilter ff_dump_extradata_bsf; >> extern const FFBitStreamFilter ff_dca_core_bsf; >> +extern const FFBitStreamFilter ff_dts2pts_bsf; >> extern const FFBitStreamFilter ff_dv_error_marker_bsf; >> extern const FFBitStreamFilter ff_eac3_core_bsf; >> extern const FFBitStreamFilter ff_extract_extradata_bsf; >> diff --git a/libavcodec/dts2pts_bsf.c b/libavcodec/dts2pts_bsf.c >> new file mode 100644 >> index 0000000000..f600150a6b >> --- /dev/null >> +++ b/libavcodec/dts2pts_bsf.c >> @@ -0,0 +1,477 @@ >> +/* >> + * Copyright (c) 2022 James Almer >> + * >> + * This file is part of FFmpeg. >> + * >> + * FFmpeg is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * FFmpeg is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with FFmpeg; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA >> + */ >> + >> +/** >> + * @file >> + * Derive PTS by reordering DTS from supported streams >> + */ >> + >> +#include "libavutil/avassert.h" >> +#include "libavutil/eval.h" >> +#include "libavutil/fifo.h" >> +#include "libavutil/opt.h" >> +#include "libavutil/tree.h" >> + >> +#include "bsf.h" >> +#include "bsf_internal.h" >> +#include "cbs.h" >> +#include "cbs_h264.h" >> +#include "h264_parse.h" >> +#include "h264_ps.h" >> + >> +typedef struct DTS2PTSNode { >> + int64_t dts; >> + int64_t duration; >> + int poc; >> +} DTS2PTSNode; >> + >> +typedef struct DTS2PTSFrame { >> + AVPacket *pkt; >> + int poc; >> + int poc_diff; >> +} DTS2PTSFrame; >> + >> +typedef struct DTS2PTSH264Context { >> + H264POCContext poc; >> + SPS sps; >> + int last_poc; >> + int highest_poc; >> + int picture_structure; >> +} DTS2PTSH264Context; >> + >> +typedef struct DTS2PTSContext { >> + struct AVTreeNode *root; >> + AVFifo *fifo; >> + >> + // Codec specific function pointers >> + int (*init)(AVBSFContext *ctx); >> + int (*filter)(AVBSFContext *ctx); >> + void (*flush)(AVBSFContext *ctx); >> + >> + CodedBitstreamContext *cbc; >> + CodedBitstreamFragment au; >> + >> + union { >> + DTS2PTSH264Context h264; >> + } u; >> + >> + int nb_frame; >> + int eof; >> +} DTS2PTSContext; >> + >> +// AVTreeNode callbacks >> +static int cmp_insert(const void *key, const void *node) >> +{ >> + return ((const DTS2PTSNode *) key)->poc - ((const DTS2PTSNode *) node)->poc; >> +} >> + >> +static int cmp_find(const void *key, const void *node) >> +{ >> + return *(const int *)key - ((const DTS2PTSNode *) node)->poc; >> +} >> + >> +static int dec_poc(void *opaque, void *elem) >> +{ >> + DTS2PTSNode *node = elem; >> + int dec = *(int *)opaque; >> + node->poc -= dec; >> + return 0; >> +} >> + >> +static int free_node(void *opaque, void *elem) >> +{ >> + DTS2PTSNode *node = elem; >> + av_free(node); >> + return 0; >> +} >> + >> +// Shared functions >> +static int alloc_and_insert_node(AVBSFContext *ctx, int64_t ts, int64_t duration, >> + int poc, int poc_diff) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + for (int i = 0; i < poc_diff; i++) { >> + struct AVTreeNode *node = av_tree_node_alloc(); >> + DTS2PTSNode *poc_node, *ret; >> + if (!node) >> + return AVERROR(ENOMEM); >> + poc_node = av_malloc(sizeof(*poc_node)); >> + if (!poc_node) { >> + av_free(node); >> + return AVERROR(ENOMEM); >> + } >> + *poc_node = (DTS2PTSNode) { ts, duration, poc++ }; >> + ret = av_tree_insert(&s->root, poc_node, cmp_insert, &node); >> + if (ret && ret != poc_node) { >> + *ret = *poc_node; >> + av_free(poc_node); >> + av_free(node); >> + } >> + } >> + return 0; >> +} >> + >> +// H.264 >> +static const CodedBitstreamUnitType h264_decompose_unit_types[] = { >> + H264_NAL_SPS, >> + H264_NAL_PPS, >> + H264_NAL_IDR_SLICE, >> + H264_NAL_SLICE, >> +}; >> + >> +static int h264_init(AVBSFContext *ctx) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + DTS2PTSH264Context *h264 = &s->u.h264; >> + >> + s->fifo = av_fifo_alloc2(H264_MAX_DPB_FRAMES, sizeof(DTS2PTSFrame), 0); > > Why do you believe that H264_MAX_DPB_FRAMES is the proper bound here? > For fields, two packets occupy one DPB slot. It seemed like a sane upper bound. I can do H264_MAX_DPB_FRAMES * 2 to better take into account fields in separate packets, like our parser propagates. > And anyway, there is no > practical bound on the number that you might have to cache: Imagine > something like the following > I0 Pn B1 B2 ... B(n-1) > A decoder only needs one reorder frame for this, because a decoder can > drop any B-frame that is has already output (if it is no longer > referenced, but the number of references is bounded, too). But a BSF > can't do this, because it has to maintain decoding order and can > therefore not discard any of the B-frames. > >> + if (!s->fifo) >> + return AVERROR(ENOMEM); >> + >> + s->cbc->decompose_unit_types = h264_decompose_unit_types; >> + s->cbc->nb_decompose_unit_types = FF_ARRAY_ELEMS(h264_decompose_unit_types); >> + >> + h264->last_poc = h264->highest_poc = INT_MIN; >> + >> + return 0; >> +} >> + >> +static int get_mmco_reset(const H264RawSliceHeader *header) >> +{ >> + if (header->nal_unit_header.nal_ref_idc == 0 || >> + !header->adaptive_ref_pic_marking_mode_flag) >> + return 0; >> + >> + for (int i = 0; i < H264_MAX_MMCO_COUNT; i++) { >> + if (header->mmco[i].memory_management_control_operation == 0) >> + return 0; >> + else if (header->mmco[i].memory_management_control_operation == 5) >> + return 1; >> + } >> + >> + return 0; >> +} >> + >> +static int h264_queue_frame(AVBSFContext *ctx, AVPacket *pkt, int poc, int *queued) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + DTS2PTSH264Context *h264 = &s->u.h264; >> + DTS2PTSFrame frame; >> + int poc_diff, ret; >> + >> + poc_diff = (h264->picture_structure == 3) + 1; >> + if (pkt->dts != AV_NOPTS_VALUE && pkt->dts < 0 && !s->nb_frame) { >> + av_tree_enumerate(s->root, &poc_diff, NULL, dec_poc); >> + s->nb_frame -= poc_diff; >> + } >> + if (poc < 0) { >> + av_tree_enumerate(s->root, &poc_diff, NULL, dec_poc); >> + s->nb_frame -= poc_diff; >> + } >> + // Check if there was a POC reset (Like an IDR slice) >> + if (s->nb_frame > h264->highest_poc) { >> + s->nb_frame = 0; >> + h264->highest_poc = h264->last_poc; >> + } >> + >> + ret = alloc_and_insert_node(ctx, pkt->dts, pkt->duration, s->nb_frame, poc_diff); >> + if (ret < 0) >> + return ret; >> + av_log(ctx, AV_LOG_DEBUG, "Queueing frame with POC %d, dts %"PRId64"\n", >> + poc, pkt->dts); >> + s->nb_frame += poc_diff; >> + >> + // Add frame to output FIFO only once >> + if (*queued) >> + return 0; >> + >> + frame = (DTS2PTSFrame) { pkt, poc, poc_diff }; >> + ret = av_fifo_write(s->fifo, &frame, 1); >> + av_assert2(ret >= 0); >> + *queued = 1; >> + >> + return 0; >> +} >> + >> +static int h264_filter(AVBSFContext *ctx) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + DTS2PTSH264Context *h264 = &s->u.h264; >> + CodedBitstreamFragment *au = &s->au; >> + AVPacket *in; >> + int output_picture_number = INT_MIN; >> + int field_poc[2]; >> + int queued = 0, ret; >> + >> + ret = ff_bsf_get_packet(ctx, &in); >> + if (ret < 0) >> + return ret; >> + >> + ret = ff_cbs_read_packet(s->cbc, au, in); >> + if (ret < 0) { >> + av_log(ctx, AV_LOG_WARNING, "Failed to parse access unit.\n"); >> + goto fail; >> + } >> + >> + for (int i = 0; i < au->nb_units; i++) { >> + CodedBitstreamUnit *unit = &au->units[i]; >> + >> + switch (unit->type) { >> + case H264_NAL_IDR_SLICE: >> + h264->poc.prev_frame_num = 0; >> + h264->poc.prev_frame_num_offset = 0; >> + h264->poc.prev_poc_msb = >> + h264->poc.prev_poc_lsb = 0; >> + // fall-through >> + case H264_NAL_SLICE: { >> + const H264RawSlice *slice = unit->content; >> + const H264RawSliceHeader *header = &slice->header; >> + const CodedBitstreamH264Context *cbs_h264 = s->cbc->priv_data; >> + const H264RawSPS *sps = cbs_h264->active_sps; >> + int got_reset; >> + >> + // Initialize the SPS struct with the fields ff_h264_init_poc() cares about >> + h264->sps.log2_max_frame_num = sps->log2_max_frame_num_minus4 + 4; >> + h264->sps.poc_type = sps->pic_order_cnt_type; >> + h264->sps.log2_max_poc_lsb = sps->log2_max_pic_order_cnt_lsb_minus4 + 4; >> + h264->sps.offset_for_non_ref_pic = sps->offset_for_non_ref_pic; >> + h264->sps.offset_for_top_to_bottom_field = sps->offset_for_top_to_bottom_field; >> + h264->sps.poc_cycle_length = sps->num_ref_frames_in_pic_order_cnt_cycle; >> + for (int i = 0; i < h264->sps.poc_cycle_length; i++) >> + h264->sps.offset_for_ref_frame[i] = sps->offset_for_ref_frame[i]; >> + >> + h264->picture_structure = sps->frame_mbs_only_flag ? 3 : >> + (header->field_pic_flag ? >> + header->field_pic_flag + header->bottom_field_flag : 3); >> + >> + h264->poc.frame_num = header->frame_num; >> + h264->poc.poc_lsb = header->pic_order_cnt_lsb; >> + h264->poc.delta_poc_bottom = header->delta_pic_order_cnt_bottom; >> + h264->poc.delta_poc[0] = header->delta_pic_order_cnt[0]; >> + h264->poc.delta_poc[1] = header->delta_pic_order_cnt[1]; >> + >> + field_poc[0] = field_poc[1] = INT_MAX; >> + ret = ff_h264_init_poc(field_poc, &output_picture_number, &h264->sps, >> + &h264->poc, h264->picture_structure, >> + header->nal_unit_header.nal_ref_idc); >> + if (ret < 0) { >> + av_log(ctx, AV_LOG_ERROR, "ff_h264_init_poc() failure\n"); >> + goto fail; >> + } >> + >> + got_reset = get_mmco_reset(header); >> + h264->poc.prev_frame_num = got_reset ? 0 : h264->poc.frame_num; >> + h264->poc.prev_frame_num_offset = got_reset ? 0 : h264->poc.frame_num_offset; >> + if (header->nal_unit_header.nal_ref_idc != 0) { >> + h264->poc.prev_poc_msb = got_reset ? 0 : h264->poc.poc_msb; >> + if (got_reset) >> + h264->poc.prev_poc_lsb = h264->picture_structure == 2 ? 0 : field_poc[0]; >> + else >> + h264->poc.prev_poc_lsb = h264->poc.poc_lsb; >> + } >> + >> + if (output_picture_number != h264->last_poc) { >> + h264->last_poc = output_picture_number; >> + h264->highest_poc = FFMAX(h264->highest_poc, output_picture_number); >> + >> + ret = h264_queue_frame(ctx, in, output_picture_number, &queued); >> + if (ret < 0) >> + goto fail; >> + } >> + } >> + default: >> + break; >> + } >> + } >> + >> + if (output_picture_number == INT_MIN) { >> + ret = AVERROR_INVALIDDATA; >> + goto fail; >> + } >> + >> + ret = AVERROR(EAGAIN); >> +fail: >> + ff_cbs_fragment_reset(au); >> + if (!queued) >> + av_packet_free(&in); >> + >> + return ret; >> +} >> + >> +static void h264_flush(AVBSFContext *ctx) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + DTS2PTSH264Context *h264 = &s->u.h264; >> + >> + memset(&h264->sps, 0, sizeof(h264->sps)); >> + memset(&h264->poc, 0, sizeof(h264->poc)); >> + h264->last_poc = h264->highest_poc = INT_MIN; >> +} >> + >> +// Core functions >> +static const struct { >> + enum AVCodecID id; >> + int (*init)(AVBSFContext *ctx); >> + int (*filter)(AVBSFContext *ctx); >> + void (*flush)(AVBSFContext *ctx); >> +} func_tab[] = { >> + { AV_CODEC_ID_H264, h264_init, h264_filter, h264_flush }, >> +}; >> + >> +static int dts2pts_init(AVBSFContext *ctx) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + CodedBitstreamFragment *au = &s->au; >> + int i, ret; >> + >> + for (i = 0; i < FF_ARRAY_ELEMS(func_tab); i++) { >> + if (func_tab[i].id == ctx->par_in->codec_id) { >> + s->init = func_tab[i].init; >> + s->filter = func_tab[i].filter; >> + s->flush = func_tab[i].flush; >> + break; >> + } >> + } >> + if (i == FF_ARRAY_ELEMS(func_tab)) >> + return AVERROR_BUG; >> + >> + ret = ff_cbs_init(&s->cbc, ctx->par_in->codec_id, ctx); >> + if (ret < 0) >> + return ret; >> + >> + ret = s->init(ctx); >> + if (ret < 0) >> + return ret; >> + >> + if (!ctx->par_in->extradata_size) >> + return 0; >> + >> + ret = ff_cbs_read_extradata(s->cbc, au, ctx->par_in); >> + if (ret < 0) >> + av_log(ctx, AV_LOG_WARNING, "Failed to parse extradata.\n"); >> + >> + ff_cbs_fragment_reset(au); >> + >> + return 0; >> +} >> + >> +static int dts2pts_filter(AVBSFContext *ctx, AVPacket *out) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + DTS2PTSNode *poc_node = NULL; >> + DTS2PTSFrame frame; >> + int poc, ret; >> + >> + // Fill up the FIFO and POC tree >> + if (!s->eof && av_fifo_can_write(s->fifo)) { >> + ret = s->filter(ctx); >> + if (ret != AVERROR_EOF) >> + return ret; >> + s->eof = 1; > > eof is only ever reset in flush, so it seems that you really intend it > to be only set on eof. That means that you intend to buffer the whole > stream in memory before returning anything. No, i don't want to buffer the whole stream, i want to fill the FIFO, whichever size it may be, before i start fetching and refilling packets one at a time. And of course eof is set on eof and reset on flush(). The only reason i added it was because i can't use FFBSFContext->eof, which is the exact same. The reason i wanted autogrow was to not have to allocate the full FIFO buffer from the start, in case a stream is short enough to not fill it, but after thinking on it i figured it was pointless since each element is small. > You presumably want to set a > huge auto-grow limit on your FIFO and that is the reason why you want to > change the behaviour of av_fifo_can_write() (I am ok with that change, > btw). But this is a horrible design: Every codec there is has an upper > bound on its DPB and therefore an upper bound on the number N of reorder > frames. This implies that if you know that you have buffered N frames > that are displayed after the first frame (according to dts, i.e. the > oldest frame) that you have currently buffered, then there can't be a > further frame that is displayed before the oldest frame (if there were, > then there would be at least N+1 reordered frames (the N + the oldest > one)). So you can already output the oldest frame (and potentially even > more frames). (Of course, for H.264, everything is complicated by the > fact that complementary field pairs only take up one slot in the DPB and > therefore only count as one reorder frame.) > As explained above, having a limit on the number of reorder frames does > not imply that there is a limit on the number of packets you need to > buffer. IMO you should impose an arbitrary, but sane limit for each > codec here. Auto-growing FIFOs are not really needed. (If you decide to > not impose an arbitrary limit, then there is no need for > av_fifo_can_write() at all.) What upper bound do you suggest, if H264_MAX_DPB_FRAMES * 2 is not enough? > >> + } >> + >> + if (!av_fifo_can_read(s->fifo)) >> + return AVERROR_EOF; >> + >> + // Fetch a packet from the FIFO >> + ret = av_fifo_read(s->fifo, &frame, 1); >> + av_assert2(ret >= 0); >> + av_packet_move_ref(out, frame.pkt); >> + av_packet_free(&frame.pkt); >> + >> + // Search the timestamp for the requested POC and set PTS >> + poc = frame.poc; >> + poc_node = av_tree_find(s->root, &poc, cmp_find, NULL); >> + if (poc_node) { >> + out->pts = poc_node->dts; >> + if (!s->eof) { >> + // Remove the found entry from the tree >> + struct AVTreeNode *node = NULL; >> + av_tree_insert(&s->root, poc_node, cmp_insert, &node); >> + av_freep(&poc_node); >> + av_free(node); >> + } >> + } else { >> + poc--; >> + if (s->eof && (poc_node = av_tree_find(s->root, &poc, cmp_find, NULL))) { >> + out->pts = poc_node->dts + poc_node->duration; >> + ret = alloc_and_insert_node(ctx, out->pts, out->duration, >> + frame.poc, frame.poc_diff); >> + if (ret < 0) { >> + av_packet_unref(out); >> + return ret; >> + } >> + if (!ret) >> + av_log(ctx, AV_LOG_DEBUG, "Queueing frame with POC %d, dts %"PRId64"\n", >> + frame.poc, out->pts); >> + } else >> + av_log(ctx, AV_LOG_WARNING, "No timestamp for POC %d in tree\n", frame.poc); >> + } >> + av_log(ctx, AV_LOG_DEBUG, "Returning frame for POC %d, dts %"PRId64", pts %"PRId64"\n", >> + frame.poc, out->dts, out->pts); >> + >> + return 0; >> +} >> + >> +static void dts2pts_flush(AVBSFContext *ctx) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + DTS2PTSFrame frame; >> + >> + s->flush(ctx); > > This presumes that every codec has a flush callback and that it has been > set. As explained below, this is not necessarily true even if the bsf is > called with a supported codec (i.e. if it does not trigger the > AVERROR_BUG codepath in init). Will add a check. > >> + s->eof = 0; >> + >> + while (av_fifo_can_read(s->fifo)) { >> + av_fifo_read(s->fifo, &frame, 1); > > while (av_fifo_read(s->fifo, &frame, 1) >= 0) > av_packet_free(&frame.pkt); > > Anyway, close is called even when init fails (or even when it was never > called; the BSF API calls it if priv_data could be allocated) and in > case the fifo has not been allocated, the above will crash as close > calls flush. Will add a check too. Ideally I'd allocate the FIFO here instead of within the coded's private init() method, but that can be done later when we add more codecs. > >> + av_packet_free(&frame.pkt); >> + } >> + >> + av_tree_enumerate(s->root, NULL, NULL, free_node); >> + av_tree_destroy(s->root); > > Missing s->root = NULL; Nice catch, will add. > >> + >> + ff_cbs_fragment_reset(&s->au); >> + ff_cbs_flush(s->cbc); >> +} >> + >> +static void dts2pts_close(AVBSFContext *ctx) >> +{ >> + DTS2PTSContext *s = ctx->priv_data; >> + >> + dts2pts_flush(ctx); >> + >> + av_fifo_freep2(&s->fifo); >> + ff_cbs_fragment_free(&s->au); >> + ff_cbs_close(&s->cbc); >> +} >> + >> +static const enum AVCodecID dts2pts_codec_ids[] = { >> + AV_CODEC_ID_H264, >> + AV_CODEC_ID_NONE, >> +}; >> + >> +const FFBitStreamFilter ff_dts2pts_bsf = { >> + .p.name = "dts2pts", >> + .p.codec_ids = dts2pts_codec_ids, >> + .priv_data_size = sizeof(DTS2PTSContext), >> + .init = dts2pts_init, >> + .flush = dts2pts_flush, >> + .close = dts2pts_close, >> + .filter = dts2pts_filter, >> +}; > > _______________________________________________ > 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". _______________________________________________ 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".