[FFmpeg-devel] [PATCH] mpeg4video: Add support for MPEG-4 Simple Studio Profile.

Michael Niedermayer michael at niedermayer.cc
Mon Jan 22 04:07:46 EET 2018


On Sun, Jan 21, 2018 at 12:37:21PM +0000, Kieran Kunhya wrote:
> On Mon, Jan 1, 2018 at 7:01 PM, Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > Hi
> 
> 

> Patch updated.
> Some of the review comments I decided not to implement in order to keep
> closer to the spec.

honestly, this reasoning makes no sense to me.
that spec does IIRC not even fully describe some parts in DPCM

also all existing mpeg&h26x code we have was designed for speed
basically ignoring if its close to some spec implementation style or not

I would prefer we try to make our code as good as we can and not try to
be similar to some specification or reference implementation.
except for maybe some niche codecs where performance totally doesnt matter


> 
> Regards,
> Kieran Kunhya

>  h263dec.c          |   10 +
>  idctdsp.c          |    7 
>  ituh263dec.c       |    9 
>  mpeg4data.h        |  116 ++++++++++++
>  mpeg4video.h       |   17 +
>  mpeg4videodec.c    |  510 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  mpegvideo.c        |   45 +++-
>  mpegvideo.h        |   17 +
>  x86/idctdsp_init.c |    1 
>  9 files changed, 714 insertions(+), 18 deletions(-)
> fb84575d9ef8a76bfe0144f7e2ec4fac2606dae0  0001-mpeg4video-Add-support-for-MPEG-4-Simple-Studio-Prof.patch
> From d9161c56787cc9b258eed6ef00b52232a648ece8 Mon Sep 17 00:00:00 2001
> From: Kieran Kunhya <kierank at obe.tv>
> Date: Fri, 29 Dec 2017 15:42:14 +0000
> Subject: [PATCH] mpeg4video: Add support for MPEG-4 Simple Studio Profile.
> 
> This is a profile supporting > 8-bit video and has a higher quality DCT
> ---
>  libavcodec/h263dec.c          |  10 +
>  libavcodec/idctdsp.c          |   7 +-
>  libavcodec/ituh263dec.c       |   9 +-
>  libavcodec/mpeg4data.h        | 116 ++++++++++
>  libavcodec/mpeg4video.h       |  17 ++
>  libavcodec/mpeg4videodec.c    | 510 +++++++++++++++++++++++++++++++++++++++++-
>  libavcodec/mpegvideo.c        |  45 +++-
>  libavcodec/mpegvideo.h        |  17 +-
>  libavcodec/x86/idctdsp_init.c |   1 +
>  9 files changed, 714 insertions(+), 18 deletions(-)
> 
> diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
> index 5608b63..39ab577 100644
> --- a/libavcodec/h263dec.c
> +++ b/libavcodec/h263dec.c
> @@ -47,6 +47,12 @@
>  
>  static enum AVPixelFormat h263_get_format(AVCodecContext *avctx)
>  {
> +    /* MPEG-4 Studio Profile only, not supported by hardware */
> +    if (avctx->bits_per_raw_sample > 8) {
> +        av_assert1(avctx->profile == FF_PROFILE_MPEG4_SIMPLE_STUDIO);
> +        return avctx->pix_fmt;
> +    }
> +
>      if (avctx->codec->id == AV_CODEC_ID_MSS2)
>          return AV_PIX_FMT_YUV420P;
>  

> @@ -197,6 +203,10 @@ static int decode_slice(MpegEncContext *s)
>  
>      ff_set_qscale(s, s->qscale);
>  
> +    if (s->studio_profile) {
> +        ff_mpeg4_decode_studio_slice_header(s->avctx->priv_data);

ignores return code



> +    }
> +
>      if (s->avctx->hwaccel) {
>          const uint8_t *start = s->gb.buffer + get_bits_count(&s->gb) / 8;
>          ret = s->avctx->hwaccel->decode_slice(s->avctx, start, s->gb.buffer_end - start);

> diff --git a/libavcodec/idctdsp.c b/libavcodec/idctdsp.c
> index 1de372d..f289785 100644
> --- a/libavcodec/idctdsp.c
> +++ b/libavcodec/idctdsp.c
> @@ -256,7 +256,12 @@ av_cold void ff_idctdsp_init(IDCTDSPContext *c, AVCodecContext *avctx)
>          c->perm_type = FF_IDCT_PERM_NONE;
>      } else {
>          if (avctx->bits_per_raw_sample == 10 || avctx->bits_per_raw_sample == 9) {
> -            c->idct_put              = ff_simple_idct_put_int16_10bit;
> +            /* 10-bit MPEG-4 Simple Studio Profile requires a higher precision IDCT
> +               However, it only uses idct_put */
> +            if (avctx->codec_id == AV_CODEC_ID_MPEG4 && avctx->profile == FF_PROFILE_MPEG4_SIMPLE_STUDIO)
> +                c->idct_put              = ff_simple_idct_put_int32_10bit;
> +            else
> +                c->idct_put              = ff_simple_idct_put_int16_10bit;

in int32 mode the functions which are not implemented should not be set.


[...]
> +/**
> + * Decode the next video packet.
> + * @return <0 if something went wrong
> + */
> +int ff_mpeg4_decode_studio_slice_header(Mpeg4DecContext *ctx)
> +{
> +    MpegEncContext *s = &ctx->m;
> +    GetBitContext *gb = &s->gb;
> +    unsigned vlc_len;
> +    uint16_t mb_num;
> +
> +    if (get_bits_long(gb, 32) == SLICE_START_CODE) {
> +        vlc_len = av_log2(((s->width + 15) / 16) * ((s->height + 15) / 16)) + 1;

> +        mb_num = get_bits(gb, vlc_len);
> +
> +        s->mb_x = mb_num % s->mb_width;
> +        s->mb_y = mb_num / s->mb_width;

missing validity checks


[...]
> +static int mpeg4_decode_studio_block(MpegEncContext *s, int32_t block[64], int n)
> +{
> +    Mpeg4DecContext *ctx = (Mpeg4DecContext *)s;

iam not sure but this could be an aliassing violation
also easy to avoid and would be cleaner too even if this is not


> +
> +    int cc, dct_dc_size, dct_diff, code, j, idx = 1, group = 0, run = 0,
> +        additional_code_len, sign, mismatch;
> +    VLC *cur_vlc = &ctx->studio_intra_tab[0];
> +    uint8_t *const scantable = s->intra_scantable.permutated;
> +    const uint16_t *quant_matrix;
> +    uint32_t flc;
> +    const int min = -1 *  (1 << (s->avctx->bits_per_raw_sample + 6));
> +    const int max =      ((1 << (s->avctx->bits_per_raw_sample + 6)) - 1);
> +
> +    mismatch = 1;
> +
> +    memset(block, 0, 64 * sizeof(int32_t));
> +
> +    if (n < 4) {
> +        cc = 0;
> +        dct_dc_size = get_vlc2(&s->gb, ctx->studio_luma_dc.table, STUDIO_INTRA_BITS, 2);
> +        quant_matrix = s->intra_matrix;
> +    }
> +    else {
> +        cc = ((n - 4) % 2) + 1; /* Table 7-30 */
> +        if (ctx->rgb)
> +            dct_dc_size = get_vlc2(&s->gb, ctx->studio_luma_dc.table, STUDIO_INTRA_BITS, 2);
> +        else
> +            dct_dc_size = get_vlc2(&s->gb, ctx->studio_chroma_dc.table, STUDIO_INTRA_BITS, 2);
> +        quant_matrix = s->chroma_intra_matrix;
> +    }
> +
> +    if (dct_dc_size == 0) {
> +        dct_diff = 0;
> +    } else {
> +        dct_diff = get_xbits(&s->gb, dct_dc_size);
> +
> +        if (dct_dc_size > 8)
> +            check_marker(s->avctx, &s->gb, "dct_dc_size > 8");
> +    }
> +
> +    s->studio_dc_val[cc] += dct_diff;
> +
> +    if (s->mpeg_quant)
> +        block[0] = s->studio_dc_val[cc] * (8 >> s->intra_dc_precision);
> +    else
> +        block[0] = s->studio_dc_val[cc] * (8 >> s->intra_dc_precision) * (8 >> s->dct_precision);
> +    // FIXME support mpeg_quant for AC coefficients
> +
> +    block[0] = av_clip(block[0], min, max);
> +    mismatch ^= block[0];
> +
> +    /* AC Coefficients */
> +    while (1) {

> +        group = get_vlc2(&s->gb, cur_vlc->table, STUDIO_INTRA_BITS, 2);
> +
> +        additional_code_len = ac_state_tab[group][0];
> +        cur_vlc = &ctx->studio_intra_tab[ac_state_tab[group][1]];

missing error checking for get_vlc2()


> +
> +        if (group == 0) {
> +            /* End of Block */
> +            break;
> +        }
> +        else if (group >= 1 && group <= 6) {
> +            /* Zero run length (Table B.47) */
> +            run = 1 << additional_code_len;
> +            if (additional_code_len)
> +                run += get_bits(&s->gb, additional_code_len);
> +            idx += run;
> +            continue;
> +        }
> +        else if (group >= 7 && group <= 12) {
> +            /* Zero run length and +/-1 level (Table B.48) */
> +            code = get_bits(&s->gb, additional_code_len);
> +            sign = code & 1;
> +            code >>= 1;
> +            run = (1 << (additional_code_len - 1)) + code;
> +            idx += run;
> +            j = scantable[idx++];
> +            block[j] = sign ? 1 : -1;
> +        }
> +        else if (group >= 13 && group <= 20) {
> +            /* Level value (Table B.49) */
> +            j = scantable[idx++];
> +            block[j] = get_xbits(&s->gb, additional_code_len);
> +        }
> +        else if (group == 21) {
> +            /* Escape */
> +            j = scantable[idx++];
> +            additional_code_len = s->avctx->bits_per_raw_sample + s->dct_precision + 4;
> +            flc = get_bits(&s->gb, additional_code_len);
> +            if (flc >> (additional_code_len-1))
> +                block[j] = -1 * (( flc ^ ((1 << additional_code_len) -1)) + 1);
> +            else
> +                block[j] = flc;
> +        }
> +        block[j] = ((8 * 2 * block[j] * quant_matrix[j] * s->qscale) >> s->dct_precision) / 32;
> +        block[j] = av_clip(block[j], min, max);
> +        mismatch ^= block[j];
> +    }
> +
> +    block[63] ^= mismatch & 1;
> +
> +    return 0;
> +}
> +

> +static int mpeg4_decode_studio_mb(MpegEncContext *s, int16_t block_[12][64])
> +{
> +    int i;
> +
> +    /* StudioMacroblock */
> +    /* Assumes I-VOP */
> +    s->mb_intra = 1;
> +    if (get_bits1(&s->gb)) { /* compression_mode */
> +        /* DCT */
> +        /* macroblock_type, 1 or 2-bit VLC */
> +        if (!get_bits1(&s->gb)) {
> +            skip_bits1(&s->gb);
> +            s->qscale = get_qscale(s);
> +        }
> +
> +        for (i = 0; i < mpeg4_block_count[s->chroma_format]; i++) {

> +            mpeg4_decode_studio_block(s, s->block32[i], i);

missing error handling

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180122/6b5e5bf5/attachment.sig>


More information about the ffmpeg-devel mailing list