[FFmpeg-devel] [PATCH v5 1/3] hevc_mp4toannexb: Insert correct parameter sets before IRAP
Andriy Gelman
andriy.gelman at gmail.com
Wed Oct 16 05:37:42 EEST 2019
Andreas,
On Thu, 01. Jan 00:00, Andreas Rheinhardt wrote:
> Andriy Gelman:
> > From: Andriy Gelman <andriy.gelman at gmail.com>
> >
> > Fixes #7799
> >
> > Currently, the mp4toannexb filter always inserts the same extradata at
> > the start of the first IRAP unit. As in ticket #7799, this can lead to
> > decoding errors if modified parameter sets are signalled in-band.
> >
> > This commit keeps track of the VPS/SPS/PPS parameter sets during the
> > conversion. The correct combination is inserted at the start of the
> > first IRAP. SEIs from extradata are inserted before each IRAP.
> >
> > This commit also makes an update to the hevc-bsf-mp4toannexb fate test
> > since the result before this patch contained duplicate parameter sets
> > in-band.
> > ---
> > libavcodec/hevc_mp4toannexb_bsf.c | 488 ++++++++++++++++++++++++++++--
> > tests/fate/hevc.mak | 2 +-
> > 2 files changed, 456 insertions(+), 34 deletions(-)
> >
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> > index 09bce5b34c..90a4d17d25 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -23,19 +23,209 @@
> >
> > #include "libavutil/intreadwrite.h"
> > #include "libavutil/mem.h"
> > +#include "libavutil/avassert.h"
> >
> > #include "avcodec.h"
> > #include "bsf.h"
> > #include "bytestream.h"
> > #include "hevc.h"
> > +#include "h2645_parse.h"
> > +#include "hevc_ps.h"
> > +#include "golomb.h"
> >
> > #define MIN_HEVCC_LENGTH 23
> > +#define PROFILE_WITHOUT_IDC_BITS 88
> > +#define IS_IRAP(s) ((s)->type >= 16 && (s)->type <= 23)
> > +#define IS_PARAMSET(s) ((s)->type >= 32 && (s)->type <= 34)
> > +
> > + /*reserved VCLs not included*/
> > +#define IS_VCL(s) ((s)->type <= 9 || ((s)->type >= 16 && (s)->type <= 21))
> > +#define HEVC_NAL_HEADER_BITS 16
> > +
> > +/*
> > + *Copies data from input buffer to output buffer. Appends annexb startcode.
> > + *out must be allocated at least out_len + in_len + 4.
> > + */
>
> A space between * and the actual comment is both common and IMO more
> readable. This goes for all comments.
>
> > +#define WRITE_NAL(out, out_len, in, in_len) do { \
> > + AV_WB32((out) + (out_len), 1); \
> > + (out_len) += 4; \
> > + memcpy((out) + (out_len), (in), (in_len)); \
> > + (out_len) += (in_len); \
> > +} while (0)
> > +
> > +typedef struct Param {
> > + uint8_t *raw_data; /*raw data to store param set payload*/
> > + int raw_size; /*size of raw_data*/
> > + size_t allocated_size; /*allocated size of raw_data*/
> > + int ref; /*stores the ref of the higher level parameter set*/
> > + int is_signalled; /*indicates whether this param has already been signalled in the cvs*/
> > +} Param;
> > +
> > +/*modified version of HEVCParamSets to store bytestream and reference to previous level*/
> > +typedef struct ParamSets {
> > + Param vps_list[HEVC_MAX_VPS_COUNT];
> > + Param sps_list[HEVC_MAX_SPS_COUNT];
> > + Param pps_list[HEVC_MAX_PPS_COUNT];
> > +
> > + Param sei; /*cached SEIs from extradata in annexb format*/
> > +} ParamSets;
> >
> > typedef struct HEVCBSFContext {
> > uint8_t length_size;
> > int extradata_parsed;
> > + ParamSets ps; /*cached VPS/SPS/PPS parameter sets + SEI from extradata*/
> > } HEVCBSFContext;
> >
> > +static int update_cached_paramset(AVBSFContext *ctx, Param *cached_param,
> > + H2645NAL *nal, int ref)
> > +{
> > + int ret;
> > +
> > + if (cached_param->raw_data && cached_param->raw_size == nal->raw_size &&
> > + !memcmp(cached_param->raw_data, nal->raw_data, nal->raw_size)) {
> > + av_log(ctx, AV_LOG_DEBUG, "NAL unit: %d. Copy already exists in parameter set.\n", nal->type);
> > + } else {
> > + if (nal->raw_size > cached_param->allocated_size) {
> > + ret = av_reallocp(&cached_param->raw_data, nal->raw_size);
> > + if (ret < 0)
> > + return ret;
> > + cached_param->allocated_size = nal->raw_size;
> > + }
> > + memcpy(cached_param->raw_data, nal->raw_data, nal->raw_size);
>
> You already include the start code in the SEI. Wouldn't it make sense
> to do the same for parameter sets? Copying would then amount to one
> big memcpy. The only complication I see is that you would have to add
> a parameter to WRITE_NAL that determines whether to write a start code
> or not.
>
> > + cached_param->raw_size = nal->raw_size;
> > + cached_param->ref = ref;
> > + cached_param->is_signalled = 0;
> > + }
> > + return 0;
> > +}
> > +
> > +static int parse_vps(AVBSFContext *ctx, H2645NAL *nal)
> > +{
> > + int vps_id, ret;
> > +
> > + HEVCBSFContext *s = ctx->priv_data;
> > + ParamSets *ps = &s->ps;
> > +
> > + GetBitContext *gb = &nal->gb;
> > + gb->index = HEVC_NAL_HEADER_BITS;
>
> One usually does not directly modify the properties of a
> GetBitContext; instead one uses the functions provided for them,
> namely skip_bits in this case. Where do you actually check that the
> various parameter sets are actually long enough?
>
> > +
> > + vps_id = get_bits(gb, 4);
> > +
> > + av_log(ctx, AV_LOG_TRACE, "Updating VPS id: %d\n", vps_id);
> > + ret = update_cached_paramset(ctx, &ps->vps_list[vps_id], nal, 0);
> > + return ret;
> > +}
> > +
> > +static int parse_sps(AVBSFContext *ctx, H2645NAL *nal)
> > +{
> > + int i, ret;
> > + int sps_id, vps_ref, max_sub_layers_minus1;
> > +
> > + HEVCBSFContext *s = ctx->priv_data;
> > + ParamSets *ps = &s->ps;
> > +
> > + uint8_t sub_layer_profile_present_flag[HEVC_MAX_SUB_LAYERS];
> > + uint8_t sub_layer_level_present_flag[HEVC_MAX_SUB_LAYERS];
> > +
> > + GetBitContext *gb = &nal->gb;
> > + gb->index = HEVC_NAL_HEADER_BITS;
> > +
> > + vps_ref = get_bits(gb, 4);
> > +
> > + max_sub_layers_minus1 = get_bits(gb, 3);
> > + skip_bits1(gb); /*sps_temporal_id_nesting_flag*/
> > + skip_bits(gb, PROFILE_WITHOUT_IDC_BITS); /*profile_tier_level*/
> > + skip_bits(gb, 8); /*general_level_idc*/
> > +
> > + for (i = 0; i < max_sub_layers_minus1; i++) {
> > + sub_layer_profile_present_flag[i] = get_bits1(gb);
> > + sub_layer_level_present_flag[i] = get_bits1(gb);
> > + }
> > +
> > + if (max_sub_layers_minus1 > 0)
> > + for (i = max_sub_layers_minus1; i < 8; i++)
> > + skip_bits(gb, 2); /*reserved_zero_2bits[i]*/
> > +
> > + for (i = 0; i < max_sub_layers_minus1; i++) {
> > + if (sub_layer_profile_present_flag[i])
> > + skip_bits(gb, PROFILE_WITHOUT_IDC_BITS); /*profile_tier_level*/
> > + if (sub_layer_level_present_flag[i])
> > + skip_bits(gb, 8); /*sub_layer_level_idc*/
> > + }
> > +
> > + /*we only need the sps_id index*/
> > + sps_id = get_ue_golomb_long(gb);
>
> This function is inherently unsafe: If the first 32 bits read are all
> zero, then the function skips 31 bits and returns the value read from
> the next 32 bits - 1. This value can be in the range
> 0..HEVC_MAX_SPS_COUNT. get_ue_golomb should satisfy all our needs
> (only the av_log(NULL is bad, but never mind.)
>
> > + if (sps_id < 0 || sps_id >= HEVC_MAX_SPS_COUNT) {
> > + av_log(ctx, AV_LOG_ERROR, "SPS id out of range: %d\n", sps_id);
> > + return AVERROR_INVALIDDATA;
> > + }
> > +
> > + av_log(ctx, AV_LOG_TRACE, "Updating SPS id: %d, VPS ref: %d\n", sps_id, vps_ref);
> > + ret = update_cached_paramset(ctx, &ps->sps_list[sps_id], nal, vps_ref);
> > + return ret;
> > +}
> > +
> > +static int parse_pps(AVBSFContext *ctx, H2645NAL *nal)
> > +{
> > + int ret;
> > + int pps_id, sps_ref;
> > +
> > + HEVCBSFContext *s = ctx->priv_data;
> > + ParamSets *ps = &s->ps;
> > +
> > + GetBitContext *gb = &nal->gb;
> > + gb->index = HEVC_NAL_HEADER_BITS;
> > +
> > + pps_id = get_ue_golomb_long(gb);
> > + if (pps_id < 0 || pps_id >= HEVC_MAX_PPS_COUNT) {
> > + av_log(ctx, AV_LOG_ERROR, "PPS id out of range: %d\n", pps_id);
> > + return AVERROR_INVALIDDATA;
> > + }
> > +
> > + sps_ref = get_ue_golomb_long(gb);
> > + if (sps_ref < 0 || sps_ref >= HEVC_MAX_SPS_COUNT) {
> > + av_log(ctx, AV_LOG_ERROR, "SPS id out of range: %d\n", sps_ref);
> > + return AVERROR_INVALIDDATA;
> > + }
> > +
> > + av_log(ctx, AV_LOG_TRACE, "Updating PPS id: %d, SPS ref: %d\n", pps_id, sps_ref);
> > + ret = update_cached_paramset(ctx, &ps->pps_list[pps_id], nal, sps_ref);
> > + return ret;
> > +}
> > +
> > +static int append_sei_annexb(Param *sei, H2645NAL *nal)
> > +{
> > + int ret;
> > +
> > + ret = av_reallocp(&sei->raw_data, sei->allocated_size + nal->raw_size + 4);
> > + if (ret < 0)
> > + return ret;
> > + sei->allocated_size += nal->raw_size + 4;
> > +
> > + WRITE_NAL(sei->raw_data, sei->raw_size, nal->raw_data, nal->raw_size);
> > + av_assert1(sei->raw_size == sei->allocated_size);
> > + return 0;
> > +}
> > +
> > +static int update_paramset(AVBSFContext *ctx, H2645NAL *nal)
> > +{
> > + int ret;
> > + switch (nal->type) {
> > + case (HEVC_NAL_VPS):
> > + if ((ret = parse_vps(ctx, nal)) < 0)
> > + return ret;
> > + break;
> > + case (HEVC_NAL_SPS):
> > + if ((ret = parse_sps(ctx, nal)) < 0)
> > + return ret;
> > + break;
> > + case (HEVC_NAL_PPS):
> > + if ((ret = parse_pps(ctx, nal)) < 0)
> > + return ret;
> > + }
> > + return 0;
> > +}
> > +
> > static int hevc_extradata_to_annexb(AVBSFContext *ctx)
> > {
> > GetByteContext gb;
> > @@ -97,84 +287,315 @@ fail:
> > static int hevc_mp4toannexb_init(AVBSFContext *ctx)
> > {
> > HEVCBSFContext *s = ctx->priv_data;
> > - int ret;
> > + H2645Packet pkt;
> > + int i, ret;
> >
> > if (ctx->par_in->extradata_size < MIN_HEVCC_LENGTH ||
> > AV_RB24(ctx->par_in->extradata) == 1 ||
> > AV_RB32(ctx->par_in->extradata) == 1) {
> > av_log(ctx, AV_LOG_VERBOSE,
> > "The input looks like it is Annex B already\n");
> > + return 0;
> > } else {
> > ret = hevc_extradata_to_annexb(ctx);
> > if (ret < 0)
> > return ret;
> > s->length_size = ret;
> > s->extradata_parsed = 1;
> > +
> > + memset(&pkt, 0, sizeof(H2645Packet));
>
> It would be better if you put the H2645Packet in this scope and
> initialized it via = { 0 }.
I'm not sure on this one because the uninit function is outside of this scope.
Let me know if you think it should be done, and I'll add it to the next
version.
I've added all the other changes.
Thanks for reviewing.
--
Andriy
More information about the ffmpeg-devel
mailing list