[FFmpeg-devel] [PATCH v6 1/3] hevc_mp4toannexb: Insert correct parameter sets before IRAP
Andriy Gelman
andriy.gelman at gmail.com
Sun Nov 24 18:29:18 EET 2019
On Sun, 24. Nov 00:02, Michael Niedermayer wrote:
> On Tue, Oct 15, 2019 at 10:50:39PM -0400, Andriy Gelman wrote:
> > 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.
> >
> > Decoding errors/visual artifacts are also present in the following fate-suite/hevc-conformance datasets for hevc->mp4->hevc conversion:
> > -RAP_B_Bossen_1.bit
> > -RPS_C_ericsson_5.bit
> > -SLIST_A_Sony_4.bit
> > -SLIST_B_Sony_8.bit
> > -SLIST_C_Sony_3.bit
> > -SLIST_D_Sony_9.bit
> > -TSKIP_A_MS_2.bit
> >
> > This commit solves these errors by keeping track of 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 | 503 ++++++++++++++++++++++++++++--
> > tests/fate/hevc.mak | 2 +-
> > 2 files changed, 472 insertions(+), 33 deletions(-)
> >
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> > index 09bce5b34c..1ca5f13807 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -23,19 +23,224 @@
> >
> > #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)
>
> This is kind of duplicated, it would be more ideal if no macros are duplicated
The same macro is defined hevdec.h. I'll use this one.
>
>
> > +#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
>
>
>
> [...]
>
> > +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;
> > +
> > + 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(gb);
> > + 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;
> > + }
> > +
> > + if (get_bits_left(gb) < 0) {
> > + av_log(ctx, AV_LOG_ERROR, "Over-read bitstream in SPS id: %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;
>
> the intermediate ret is useless here, the same issue occurs in other cases too
ok, will update.
> [...]
>
> > +
> > static int hevc_extradata_to_annexb(AVBSFContext *ctx)
> > {
> > GetByteContext gb;
> > @@ -97,6 +302,7 @@ fail:
> > static int hevc_mp4toannexb_init(AVBSFContext *ctx)
> > {
> > HEVCBSFContext *s = ctx->priv_data;
> > + H2645Packet pkt;
> > int ret;
> >
> > if (ctx->par_in->extradata_size < MIN_HEVCC_LENGTH ||
> > @@ -104,77 +310,309 @@ static int hevc_mp4toannexb_init(AVBSFContext *ctx)
> > 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));
> > + ret = ff_h2645_packet_split(&pkt, ctx->par_out->extradata, ctx->par_out->extradata_size,
> > + ctx, 0, 0, AV_CODEC_ID_HEVC, 1, 0);
> > + if (ret < 0)
> > + goto done;
> > +
> > + for (int i = 0; i < pkt.nb_nals; ++i) {
> > + H2645NAL *nal = &pkt.nals[i];
> > +
>
> > + /*current segmentation algorithm includes next 0x00 from next nal unit*/
> > + if (nal->raw_data[nal->raw_size - 1] == 0x00)
>
> Can raw_size be 0 here ?
> a check or assert on raw_size > 0 might make this more robust
ff_h2645_packet_split throws away any packets where rbsp_size <= 0 (h2645_parse.c:508). Because raw_size >= rbsp_size, implies raw_size > 0.
I'll add av_assert0 as you suggested.
Also patch 3/3 removes this line altogether by using idea that extradata is in
HVCC format and there is no need to segment by searching for the startcode.
I can squash if you think it's best.
>
> [...]
> > +/*
> > + * Function converts mp4 access unit into annexb
> > + * Output packet structure
> > + * VPS, SPS, PPS, [SEI(from extradata)], [SEI_PREFIX(from access unit)], IRAP, [SEI_SUFFIX]
> > + * or
> > + * [SEI_PREFIX (from access unit)], [PPS (if not already signalled)], VCL(non-irap), [SEI_SUFFIX]
> > + */
> > static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
> > {
> > HEVCBSFContext *s = ctx->priv_data;
> > AVPacket *in;
> > - GetByteContext gb;
> > -
> > - int got_irap = 0;
> > - int i, ret = 0;
> > + H2645Packet pkt;
> > + int i, j, prev_size, ret;
> > + int irap_done;
> >
> > ret = ff_bsf_get_packet(ctx, &in);
> > if (ret < 0)
> > return ret;
> >
> > + /* output the annexb nalu if extradata is not parsed*/
> > if (!s->extradata_parsed) {
> > av_packet_move_ref(out, in);
> > av_packet_free(&in);
> > return 0;
> > }
> >
>
> > - bytestream2_init(&gb, in->data, in->size);
>
> Is this really better without using an existing bytestream* API ?
If I use the API, then I'd have to call bytestraem2_init for each nal unit. I
also don't use the bytestream2_get_byte function. It seemed simpler to use the WRITE_NAL macro.
But maybe I've misunderstood your question.
>
> Also as you mentioned, the nuh_layer_id != 0 issue which mkver found.
> That also should be fixed
I'll add an option to ff_h2645_packet_split to not remove such packets.
Thank you very much for reviewing.
--
Andriy
More information about the ffmpeg-devel
mailing list