From patchwork Tue Jan 26 23:06:20 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Mark Thompson X-Patchwork-Id: 25211 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 4D86544A194 for ; Wed, 27 Jan 2021 01:06:30 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1A5AA6880DA; Wed, 27 Jan 2021 01:06:30 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D1361680459 for ; Wed, 27 Jan 2021 01:06:22 +0200 (EET) Received: by mail-wm1-f48.google.com with SMTP id c127so4101228wmf.5 for ; Tue, 26 Jan 2021 15:06:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=GSnPKGTt481qWmTW4QBynPZxTLzNr+KI39ZE0bKBq50=; b=cTf384mzPLkShyUiK7Ccyne+Oy2aPEgoAnP06f4dQgsLYJC2TrZk93AXhtDUtL482X YiYtVZpBjNudkbcYXQ38Jm+Zop8yYo6SlI9lCuel9erqNq8kax1bVHOHRa54nhNBchBm jvxCvS5aPn8s+yENX9O9+aVnTtcerNRM+LGkuEQXMyF24rVagUmpiAtk0NCqA4DA9Gp9 tzzPLkzGppU2f/tfjqyQKpE2SiqZzm72IIvlL/bqDW9QYs4VXCY9Gxbd2OKErF0+D7ZG XkapfvjF8LfYxWuZeEbCY0wfBXM/Q62CF1NWbHGzavYb5wsJwPTjvJiewkanqLs//ksd cv2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=GSnPKGTt481qWmTW4QBynPZxTLzNr+KI39ZE0bKBq50=; b=tCqbcuQOEkwYNb0sJUlVd6vrxfQ+waLdBkiDmojO/U4a6/PENdmaP5yhj8vrU1WRIK aPEMB0mNFIXw8aqmy176MXw34r3Va+QlYqYy7hqWhxELWzZ6xnUWKL1PF9B5mAbNSUAl DLzWlMNVVNVSSRDaXtzNIA53mG4TaaIZYeixuHJW/4R54eqhBpiheo0t1h0E0Oq4VhDx mjsdIidbIQsJVWBzIvFqNEgOw4WrqWNPK1obVpVpxnwTY3T6IOzZhtIi3AD9E40sf5j+ kQjkhE6fe43cPcIxUYBAJsRGDvO54uXxbAQoNsg+5yN+qkbarL43WhDFMi2TtQH/SPRr zDlQ== X-Gm-Message-State: AOAM5315xXbubQuOep5RgDxe+Thab9WSyPQxsCLBMjtUdGTJqu1eEXk8 LINtJzxdug3bxz+O6R2dn6lBCOmrsE4AEA== X-Google-Smtp-Source: ABdhPJzg5AOo12A5k6PAVCXK2a6Oj8zVvOfXBU2i8ym1LAT5AJUwyPmhpW/JdIk1rMXyWIewTGUNPA== X-Received: by 2002:a1c:545d:: with SMTP id p29mr1628598wmi.2.1611702381788; Tue, 26 Jan 2021 15:06:21 -0800 (PST) Received: from [192.168.0.3] (cpc91226-cmbg18-2-0-cust7.5-4.cable.virginm.net. [82.0.29.8]) by smtp.gmail.com with ESMTPSA id b69sm76445wmb.36.2021.01.26.15.06.20 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 26 Jan 2021 15:06:21 -0800 (PST) To: ffmpeg-devel@ffmpeg.org References: <20210111152357.3965-1-nuomi2021@gmail.com> <20210125141507.11012-1-nuomi2021@gmail.com> <20210125141507.11012-5-nuomi2021@gmail.com> <6f14b9d5-377b-21af-b938-74a3a253cd13@gmail.com> From: Mark Thompson Message-ID: <863c268b-cb82-e0c0-9bb9-f83b04055e06@jkqxz.net> Date: Tue, 26 Jan 2021 23:06:20 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Subject: [FFmpeg-devel] [PATCH] cbs_h2645: Implement replace-PS with a table rather than many functions X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" --- On 26/01/2021 13:40, James Almer wrote: > On 1/26/2021 10:35 AM, Nuo Mi wrote: >> On Tue, Jan 26, 2021 at 5:54 AM James Almer wrote: >> >>> On 1/25/2021 2:12 PM, James Almer wrote: >>>> Did you add this just to set active_vps? I thought the >>>> idea was to remove all that since it's not a concept defined in h266. >>> >>> The following should get rid of active_*, but the actual fields can't be >>> removed from CodedBitstreamH266Context unless we duplicate the >>> cbs_h2645_replace_ps() macro to remove the relevant lines it shares with >>> the other two codecs. >>> >> One solution  is  define a cbs_h26n_replace_ps. >> cbs_h2645 _replace_ps calls  cbs_h26n_replace_ps and set active_xps to NULL. >> cbs_h266_replace_ps calls the cbs_h26n_replace_ps only. >> I did not use this since I think active_xps will make the caller's life >> easier. >> They do not need to use the id again and again to get pps, sps, vps. >> If you think remove it is better, I can remove it. > > I'll leave that decision to Mark. I know he tries to keep CBS as close as the spec as possible, which is why i assumed removing active_*ps for h266 felt like the proper thing to do. I think I have to agree with that. On messing with all those replace-PS functions, how would you like something like this to put the information in a table rather than generating lots of functions? Further thoughts: * active_offset can be set to a special value for H.266 to mean "active not a thing here". * The table definition might be better with a carefully chosen macro. * The replace_ps calls are actually all the same and depend only on the NAL unit type, so can we move them out of the switch? Thanks, - Mark libavcodec/cbs_h2645.c | 171 +++++++++++++++++++++++++++++++---------- 1 file changed, 130 insertions(+), 41 deletions(-) diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 6005d46e0d..36212d1da6 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -661,38 +661,127 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx, return 0; } -#define cbs_h2645_replace_ps(h26n, ps_name, ps_var, id_element) \ -static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \ - CodedBitstreamUnit *unit) \ -{ \ - CodedBitstreamH26 ## h26n ## Context *priv = ctx->priv_data; \ - H26 ## h26n ## Raw ## ps_name *ps_var = unit->content; \ - unsigned int id = ps_var->id_element; \ - int err; \ - if (id >= FF_ARRAY_ELEMS(priv->ps_var)) { \ - av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid " #ps_name \ - " id : %d.\n", id); \ - return AVERROR_INVALIDDATA; \ - } \ - err = ff_cbs_make_unit_refcounted(ctx, unit); \ - if (err < 0) \ - return err; \ - if (priv->ps_var[id] == priv->active_ ## ps_var) \ - priv->active_ ## ps_var = NULL ; \ - av_buffer_unref(&priv->ps_var ## _ref[id]); \ - av_assert0(unit->content_ref); \ - priv->ps_var ## _ref[id] = av_buffer_ref(unit->content_ref); \ - if (!priv->ps_var ## _ref[id]) \ - return AVERROR(ENOMEM); \ - priv->ps_var[id] = (H26 ## h26n ## Raw ## ps_name *)priv->ps_var ## _ref[id]->data; \ - return 0; \ -} +static int cbs_h2645_replace_ps(CodedBitstreamContext *ctx, + CodedBitstreamUnit *unit) +{ + typedef struct { + int nal_unit_type; + int max_count; + const char *name; + size_t id_offset; + size_t ref_array_offset; + size_t ptr_array_offset; + size_t active_offset; + } PSType; + + static const PSType h264_ps_types[] = { + { + H264_NAL_SPS, + H264_MAX_SPS_COUNT, + "SPS", + offsetof(H264RawSPS, seq_parameter_set_id), + offsetof(CodedBitstreamH264Context, sps_ref), + offsetof(CodedBitstreamH264Context, sps), + offsetof(CodedBitstreamH264Context, active_sps), + }, + { + H264_NAL_PPS, + H264_MAX_PPS_COUNT, + "PPS", + offsetof(H264RawPPS, pic_parameter_set_id), + offsetof(CodedBitstreamH264Context, pps_ref), + offsetof(CodedBitstreamH264Context, pps), + offsetof(CodedBitstreamH264Context, active_pps), + }, + }; + + static const PSType h265_ps_types[] = { + { + HEVC_NAL_VPS, + HEVC_MAX_VPS_COUNT, + "VPS", + offsetof(H265RawVPS, vps_video_parameter_set_id), + offsetof(CodedBitstreamH265Context, vps_ref), + offsetof(CodedBitstreamH265Context, vps), + offsetof(CodedBitstreamH265Context, active_vps), + }, + { + HEVC_NAL_SPS, + HEVC_MAX_SPS_COUNT, + "SPS", + offsetof(H265RawSPS, sps_seq_parameter_set_id), + offsetof(CodedBitstreamH265Context, sps_ref), + offsetof(CodedBitstreamH265Context, sps), + offsetof(CodedBitstreamH265Context, active_sps), + }, + { + HEVC_NAL_PPS, + HEVC_MAX_PPS_COUNT, + "PPS", + offsetof(H265RawPPS, pps_pic_parameter_set_id), + offsetof(CodedBitstreamH265Context, pps_ref), + offsetof(CodedBitstreamH265Context, pps), + offsetof(CodedBitstreamH265Context, active_pps), + }, + }; -cbs_h2645_replace_ps(4, SPS, sps, seq_parameter_set_id) -cbs_h2645_replace_ps(4, PPS, pps, pic_parameter_set_id) -cbs_h2645_replace_ps(5, VPS, vps, vps_video_parameter_set_id) -cbs_h2645_replace_ps(5, SPS, sps, sps_seq_parameter_set_id) -cbs_h2645_replace_ps(5, PPS, pps, pps_pic_parameter_set_id) + const PSType *ps_type; + AVBufferRef **ref_array; + void **ptr_array; + void **active; + int err, id, i, nb_ps_types; + + switch (ctx->codec->codec_id) { + case AV_CODEC_ID_H264: + ps_type = h264_ps_types; + nb_ps_types = FF_ARRAY_ELEMS(h264_ps_types); + break; + case AV_CODEC_ID_H265: + ps_type = h265_ps_types; + nb_ps_types = FF_ARRAY_ELEMS(h265_ps_types); + break; + default: + av_assert0(0); + } + + for (i = 0; i < nb_ps_types; i++) { + if (ps_type->nal_unit_type == unit->type) + break; + ++ps_type; + } + av_assert0(i < nb_ps_types); + + id = *((uint8_t*)unit->content + ps_type->id_offset); + + if (id >= ps_type->max_count) { + av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid %s id: %d.\n", + ps_type->name, id); + return AVERROR_INVALIDDATA; + } + + err = ff_cbs_make_unit_refcounted(ctx, unit); + if (err < 0) + return err; + + ref_array = + (AVBufferRef**)((uint8_t*)ctx->priv_data + ps_type->ref_array_offset); + ptr_array = (void**)((uint8_t*)ctx->priv_data + ps_type->ptr_array_offset); + active = (void**)((uint8_t*)ctx->priv_data + ps_type->active_offset); + + if (ptr_array[id] == *active) { + // The old active parameter set is being overwritten, so it can't + // be active after this point. + *active = NULL; + } + av_buffer_unref(&ref_array[id]); + + ref_array[id] = av_buffer_ref(unit->content_ref); + if (!ref_array[id]) + return AVERROR(ENOMEM); + ptr_array[id] = ref_array[id]->data; + + return 0; +} static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx, CodedBitstreamUnit *unit) @@ -717,7 +806,7 @@ static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h264_replace_sps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -739,7 +828,7 @@ static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h264_replace_pps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -836,7 +925,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h265_replace_vps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -849,7 +938,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h265_replace_sps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -863,7 +952,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h265_replace_pps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -1007,7 +1096,7 @@ static int cbs_h264_write_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h264_replace_sps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -1031,7 +1120,7 @@ static int cbs_h264_write_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h264_replace_pps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -1124,7 +1213,7 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h265_replace_vps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -1138,7 +1227,7 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h265_replace_sps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; } @@ -1152,7 +1241,7 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, if (err < 0) return err; - err = cbs_h265_replace_pps(ctx, unit); + err = cbs_h2645_replace_ps(ctx, unit); if (err < 0) return err; }