[FFmpeg-devel] avcodec/vp9_parser: parse size and colorspace values from frame headers

Submitted by James Almer on Jan. 19, 2018, 3:51 a.m.

Details

Message ID 20180119035121.4612-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Jan. 19, 2018, 3:51 a.m.
Improves remuxing support when vp9 decoder is not compiled in.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/vp9_parser.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Jan. 19, 2018, 11:19 a.m.
2018-01-19 4:51 GMT+01:00 James Almer <jamrial@gmail.com>:
> Improves remuxing support when vp9 decoder is not compiled in.

> +static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
> +                                   GetBitContext *gb)

Isn't this a duplicate of an existing, non-trivial function?

Carl Eugen
James Almer Jan. 19, 2018, 4:09 p.m.
On 1/19/2018 8:19 AM, Carl Eugen Hoyos wrote:
> 2018-01-19 4:51 GMT+01:00 James Almer <jamrial@gmail.com>:
>> Improves remuxing support when vp9 decoder is not compiled in.
> 
>> +static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
>> +                                   GetBitContext *gb)
> 
> Isn't this a duplicate of an existing, non-trivial function?
> 
> Carl Eugen

Yes, but just like how h264/hevc have duplicate NAL parsing code all
across the codebase, it's different enough that making it shared would
be too complex/dirty to be worth the hassle.
Carl Eugen Hoyos Jan. 19, 2018, 6:13 p.m.
2018-01-19 17:09 GMT+01:00 James Almer <jamrial@gmail.com>:
> On 1/19/2018 8:19 AM, Carl Eugen Hoyos wrote:
>> 2018-01-19 4:51 GMT+01:00 James Almer <jamrial@gmail.com>:
>>> Improves remuxing support when vp9 decoder is not compiled in.
>>
>>> +static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
>>> +                                   GetBitContext *gb)
>>
>> Isn't this a duplicate of an existing, non-trivial function?
>>
>> Carl Eugen
>
> Yes, but just like how h264/hevc have duplicate NAL parsing code all
> across the codebase, it's different enough that making it shared would
> be too complex/dirty to be worth the hassle.

I wish this argument would count more often.

Thank you, Carl Eugen
Michael Niedermayer Jan. 19, 2018, 11:56 p.m.
On Fri, Jan 19, 2018 at 12:51:21AM -0300, James Almer wrote:
> Improves remuxing support when vp9 decoder is not compiled in.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/vp9_parser.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> index 9531f34a32..b059477747 100644
> --- a/libavcodec/vp9_parser.c
> +++ b/libavcodec/vp9_parser.c
> @@ -23,15 +23,69 @@
>  
>  #include "libavutil/intreadwrite.h"
>  #include "libavcodec/get_bits.h"
> +#include "libavcodec/internal.h"
>  #include "parser.h"
>  
> +#define VP9_SYNCCODE 0x498342
> +
> +static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
> +                                   GetBitContext *gb)
> +{
> +    static const enum AVColorSpace colorspaces[8] = {
> +        AVCOL_SPC_UNSPECIFIED, AVCOL_SPC_BT470BG, AVCOL_SPC_BT709, AVCOL_SPC_SMPTE170M,
> +        AVCOL_SPC_SMPTE240M, AVCOL_SPC_BT2020_NCL, AVCOL_SPC_RESERVED, AVCOL_SPC_RGB,
> +    };
> +    int bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb); // 0:8, 1:10, 2:12
> +
> +    avctx->colorspace = colorspaces[get_bits(gb, 3)];
> +    if (avctx->colorspace == AVCOL_SPC_RGB) { // RGB = profile 1
> +        static const enum AVPixelFormat pix_fmt_rgb[3] = {
> +            AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12
> +        };
> +        if (avctx->profile & 1) {
> +            if (get_bits1(gb)) // reserved bit
> +                return AVERROR_INVALIDDATA;
> +        } else
> +            return AVERROR_INVALIDDATA;
> +        avctx->color_range = AVCOL_RANGE_JPEG;
> +        ctx->format = pix_fmt_rgb[bits];
> +    } else {
> +        static const enum AVPixelFormat pix_fmt_for_ss[3][2 /* v */][2 /* h */] = {
> +            { { AV_PIX_FMT_YUV444P,   AV_PIX_FMT_YUV422P },
> +              { AV_PIX_FMT_YUV440P,   AV_PIX_FMT_YUV420P } },
> +            { { AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV422P10 },
> +              { AV_PIX_FMT_YUV440P10, AV_PIX_FMT_YUV420P10 } },
> +            { { AV_PIX_FMT_YUV444P12, AV_PIX_FMT_YUV422P12 },
> +              { AV_PIX_FMT_YUV440P12, AV_PIX_FMT_YUV420P12 } }
> +        };
> +        avctx->color_range = get_bits1(gb) ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> +        if (avctx->profile & 1) {
> +            int ss_h, ss_v, format;
> +
> +            ss_h = get_bits1(gb);
> +            ss_v = get_bits1(gb);
> +            format = pix_fmt_for_ss[bits][ss_v][ss_h];
> +            if (format == AV_PIX_FMT_YUV420P)
> +                // YUV 4:2:0 not supported in profiles 1 and 3
> +                return AVERROR_INVALIDDATA;
> +            else if (get_bits1(gb)) // color details reserved bit
> +                return AVERROR_INVALIDDATA;
> +            ctx->format = format;
> +        } else {
> +            ctx->format = pix_fmt_for_ss[bits][1][1];
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int parse(AVCodecParserContext *ctx,
>                   AVCodecContext *avctx,
>                   const uint8_t **out_data, int *out_size,
>                   const uint8_t *data, int size)
>  {
>      GetBitContext gb;
> -    int res, profile, keyframe;
> +    int res, profile, keyframe, invisible, errorres;
>  
>      *out_data = data;
>      *out_size = size;
> @@ -42,21 +96,63 @@ static int parse(AVCodecParserContext *ctx,
>      profile  = get_bits1(&gb);
>      profile |= get_bits1(&gb) << 1;
>      if (profile == 3) profile += get_bits1(&gb);
> +    if (profile > 3)
> +        return size;
>  
>      if (get_bits1(&gb)) {
>          keyframe = 0;
> +        skip_bits(&gb, 3);
>      } else {
>          keyframe  = !get_bits1(&gb);
>      }
>  
> +    invisible = !get_bits1(&gb);
> +    errorres  = get_bits1(&gb);
> +
>      if (!keyframe) {
> +        int intraonly = invisible ? get_bits1(&gb) : 0;
> +        if (!errorres)
> +            skip_bits(&gb, 2);
> +        if (intraonly) {
> +            if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
> +                return size;
> +            avctx->profile = profile;
> +            if (profile >= 1) {
> +                if ((read_colorspace_details(ctx, avctx, &gb)) < 0)
> +                    return size;
> +            } else {
> +                ctx->format        = AV_PIX_FMT_YUV420P;
> +                avctx->colorspace  = AVCOL_SPC_BT470BG;
> +                avctx->color_range = AVCOL_RANGE_MPEG;
> +            }
> +            skip_bits(&gb, 8); // refreshrefmask
> +            ctx->width  = get_bits(&gb, 16) + 1;
> +            ctx->height = get_bits(&gb, 16) + 1;
> +            if (get_bits1(&gb)) // display size
> +                skip_bits(&gb, 32);
> +        }
> +
>          ctx->pict_type = AV_PICTURE_TYPE_P;
>          ctx->key_frame = 0;
>      } else {
> +        if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
> +            return size;
> +        avctx->profile = profile;
> +        if (read_colorspace_details(ctx, avctx, &gb) < 0)
> +            return size;
> +        ctx->width  = get_bits(&gb, 16) + 1;
> +        ctx->height = get_bits(&gb, 16) + 1;
> +        if (get_bits1(&gb)) // display size
> +            skip_bits(&gb, 32);
> +
>          ctx->pict_type = AV_PICTURE_TYPE_I;
>          ctx->key_frame = 1;
>      }
>  

> +    if (ctx->width && (!avctx->width || !avctx->height ||
> +                       !avctx->coded_width || !avctx->coded_height))
> +        ff_set_dimensions(avctx, ctx->width, ctx->height);

This is the only ff_set_dimensions() in the codebase with non fixed parameters
that ignores the return value 
is that intended ?

[...]
James Almer Jan. 20, 2018, 12:33 a.m.
On 1/19/2018 8:56 PM, Michael Niedermayer wrote:
> On Fri, Jan 19, 2018 at 12:51:21AM -0300, James Almer wrote:
>> Improves remuxing support when vp9 decoder is not compiled in.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/vp9_parser.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 97 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
>> index 9531f34a32..b059477747 100644
>> --- a/libavcodec/vp9_parser.c
>> +++ b/libavcodec/vp9_parser.c
>> @@ -23,15 +23,69 @@
>>  
>>  #include "libavutil/intreadwrite.h"
>>  #include "libavcodec/get_bits.h"
>> +#include "libavcodec/internal.h"
>>  #include "parser.h"
>>  
>> +#define VP9_SYNCCODE 0x498342
>> +
>> +static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
>> +                                   GetBitContext *gb)
>> +{
>> +    static const enum AVColorSpace colorspaces[8] = {
>> +        AVCOL_SPC_UNSPECIFIED, AVCOL_SPC_BT470BG, AVCOL_SPC_BT709, AVCOL_SPC_SMPTE170M,
>> +        AVCOL_SPC_SMPTE240M, AVCOL_SPC_BT2020_NCL, AVCOL_SPC_RESERVED, AVCOL_SPC_RGB,
>> +    };
>> +    int bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb); // 0:8, 1:10, 2:12
>> +
>> +    avctx->colorspace = colorspaces[get_bits(gb, 3)];
>> +    if (avctx->colorspace == AVCOL_SPC_RGB) { // RGB = profile 1
>> +        static const enum AVPixelFormat pix_fmt_rgb[3] = {
>> +            AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12
>> +        };
>> +        if (avctx->profile & 1) {
>> +            if (get_bits1(gb)) // reserved bit
>> +                return AVERROR_INVALIDDATA;
>> +        } else
>> +            return AVERROR_INVALIDDATA;
>> +        avctx->color_range = AVCOL_RANGE_JPEG;
>> +        ctx->format = pix_fmt_rgb[bits];
>> +    } else {
>> +        static const enum AVPixelFormat pix_fmt_for_ss[3][2 /* v */][2 /* h */] = {
>> +            { { AV_PIX_FMT_YUV444P,   AV_PIX_FMT_YUV422P },
>> +              { AV_PIX_FMT_YUV440P,   AV_PIX_FMT_YUV420P } },
>> +            { { AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV422P10 },
>> +              { AV_PIX_FMT_YUV440P10, AV_PIX_FMT_YUV420P10 } },
>> +            { { AV_PIX_FMT_YUV444P12, AV_PIX_FMT_YUV422P12 },
>> +              { AV_PIX_FMT_YUV440P12, AV_PIX_FMT_YUV420P12 } }
>> +        };
>> +        avctx->color_range = get_bits1(gb) ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
>> +        if (avctx->profile & 1) {
>> +            int ss_h, ss_v, format;
>> +
>> +            ss_h = get_bits1(gb);
>> +            ss_v = get_bits1(gb);
>> +            format = pix_fmt_for_ss[bits][ss_v][ss_h];
>> +            if (format == AV_PIX_FMT_YUV420P)
>> +                // YUV 4:2:0 not supported in profiles 1 and 3
>> +                return AVERROR_INVALIDDATA;
>> +            else if (get_bits1(gb)) // color details reserved bit
>> +                return AVERROR_INVALIDDATA;
>> +            ctx->format = format;
>> +        } else {
>> +            ctx->format = pix_fmt_for_ss[bits][1][1];
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static int parse(AVCodecParserContext *ctx,
>>                   AVCodecContext *avctx,
>>                   const uint8_t **out_data, int *out_size,
>>                   const uint8_t *data, int size)
>>  {
>>      GetBitContext gb;
>> -    int res, profile, keyframe;
>> +    int res, profile, keyframe, invisible, errorres;
>>  
>>      *out_data = data;
>>      *out_size = size;
>> @@ -42,21 +96,63 @@ static int parse(AVCodecParserContext *ctx,
>>      profile  = get_bits1(&gb);
>>      profile |= get_bits1(&gb) << 1;
>>      if (profile == 3) profile += get_bits1(&gb);
>> +    if (profile > 3)
>> +        return size;
>>  
>>      if (get_bits1(&gb)) {
>>          keyframe = 0;
>> +        skip_bits(&gb, 3);
>>      } else {
>>          keyframe  = !get_bits1(&gb);
>>      }
>>  
>> +    invisible = !get_bits1(&gb);
>> +    errorres  = get_bits1(&gb);
>> +
>>      if (!keyframe) {
>> +        int intraonly = invisible ? get_bits1(&gb) : 0;
>> +        if (!errorres)
>> +            skip_bits(&gb, 2);
>> +        if (intraonly) {
>> +            if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
>> +                return size;
>> +            avctx->profile = profile;
>> +            if (profile >= 1) {
>> +                if ((read_colorspace_details(ctx, avctx, &gb)) < 0)
>> +                    return size;
>> +            } else {
>> +                ctx->format        = AV_PIX_FMT_YUV420P;
>> +                avctx->colorspace  = AVCOL_SPC_BT470BG;
>> +                avctx->color_range = AVCOL_RANGE_MPEG;
>> +            }
>> +            skip_bits(&gb, 8); // refreshrefmask
>> +            ctx->width  = get_bits(&gb, 16) + 1;
>> +            ctx->height = get_bits(&gb, 16) + 1;
>> +            if (get_bits1(&gb)) // display size
>> +                skip_bits(&gb, 32);
>> +        }
>> +
>>          ctx->pict_type = AV_PICTURE_TYPE_P;
>>          ctx->key_frame = 0;
>>      } else {
>> +        if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
>> +            return size;
>> +        avctx->profile = profile;
>> +        if (read_colorspace_details(ctx, avctx, &gb) < 0)
>> +            return size;
>> +        ctx->width  = get_bits(&gb, 16) + 1;
>> +        ctx->height = get_bits(&gb, 16) + 1;
>> +        if (get_bits1(&gb)) // display size
>> +            skip_bits(&gb, 32);
>> +
>>          ctx->pict_type = AV_PICTURE_TYPE_I;
>>          ctx->key_frame = 1;
>>      }
>>  
> 
>> +    if (ctx->width && (!avctx->width || !avctx->height ||
>> +                       !avctx->coded_width || !avctx->coded_height))
>> +        ff_set_dimensions(avctx, ctx->width, ctx->height);
> 
> This is the only ff_set_dimensions() in the codebase with non fixed parameters
> that ignores the return value 
> is that intended ?

Yes, since right after it the only line that remains in the entire
function is the return.

I can change it to "if (ff_set_dimensions() < 0) return size" if that's
preferred, but it will be functionally the same since parsers can't
return errors, so they always return the full frame size.
Michael Niedermayer Jan. 20, 2018, 12:36 a.m.
On Fri, Jan 19, 2018 at 09:33:46PM -0300, James Almer wrote:
> On 1/19/2018 8:56 PM, Michael Niedermayer wrote:
> > On Fri, Jan 19, 2018 at 12:51:21AM -0300, James Almer wrote:
> >> Improves remuxing support when vp9 decoder is not compiled in.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>  libavcodec/vp9_parser.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 97 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> >> index 9531f34a32..b059477747 100644
> >> --- a/libavcodec/vp9_parser.c
> >> +++ b/libavcodec/vp9_parser.c
> >> @@ -23,15 +23,69 @@
> >>  
> >>  #include "libavutil/intreadwrite.h"
> >>  #include "libavcodec/get_bits.h"
> >> +#include "libavcodec/internal.h"
> >>  #include "parser.h"
> >>  
> >> +#define VP9_SYNCCODE 0x498342
> >> +
> >> +static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
> >> +                                   GetBitContext *gb)
> >> +{
> >> +    static const enum AVColorSpace colorspaces[8] = {
> >> +        AVCOL_SPC_UNSPECIFIED, AVCOL_SPC_BT470BG, AVCOL_SPC_BT709, AVCOL_SPC_SMPTE170M,
> >> +        AVCOL_SPC_SMPTE240M, AVCOL_SPC_BT2020_NCL, AVCOL_SPC_RESERVED, AVCOL_SPC_RGB,
> >> +    };
> >> +    int bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb); // 0:8, 1:10, 2:12
> >> +
> >> +    avctx->colorspace = colorspaces[get_bits(gb, 3)];
> >> +    if (avctx->colorspace == AVCOL_SPC_RGB) { // RGB = profile 1
> >> +        static const enum AVPixelFormat pix_fmt_rgb[3] = {
> >> +            AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12
> >> +        };
> >> +        if (avctx->profile & 1) {
> >> +            if (get_bits1(gb)) // reserved bit
> >> +                return AVERROR_INVALIDDATA;
> >> +        } else
> >> +            return AVERROR_INVALIDDATA;
> >> +        avctx->color_range = AVCOL_RANGE_JPEG;
> >> +        ctx->format = pix_fmt_rgb[bits];
> >> +    } else {
> >> +        static const enum AVPixelFormat pix_fmt_for_ss[3][2 /* v */][2 /* h */] = {
> >> +            { { AV_PIX_FMT_YUV444P,   AV_PIX_FMT_YUV422P },
> >> +              { AV_PIX_FMT_YUV440P,   AV_PIX_FMT_YUV420P } },
> >> +            { { AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV422P10 },
> >> +              { AV_PIX_FMT_YUV440P10, AV_PIX_FMT_YUV420P10 } },
> >> +            { { AV_PIX_FMT_YUV444P12, AV_PIX_FMT_YUV422P12 },
> >> +              { AV_PIX_FMT_YUV440P12, AV_PIX_FMT_YUV420P12 } }
> >> +        };
> >> +        avctx->color_range = get_bits1(gb) ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> >> +        if (avctx->profile & 1) {
> >> +            int ss_h, ss_v, format;
> >> +
> >> +            ss_h = get_bits1(gb);
> >> +            ss_v = get_bits1(gb);
> >> +            format = pix_fmt_for_ss[bits][ss_v][ss_h];
> >> +            if (format == AV_PIX_FMT_YUV420P)
> >> +                // YUV 4:2:0 not supported in profiles 1 and 3
> >> +                return AVERROR_INVALIDDATA;
> >> +            else if (get_bits1(gb)) // color details reserved bit
> >> +                return AVERROR_INVALIDDATA;
> >> +            ctx->format = format;
> >> +        } else {
> >> +            ctx->format = pix_fmt_for_ss[bits][1][1];
> >> +        }
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>  static int parse(AVCodecParserContext *ctx,
> >>                   AVCodecContext *avctx,
> >>                   const uint8_t **out_data, int *out_size,
> >>                   const uint8_t *data, int size)
> >>  {
> >>      GetBitContext gb;
> >> -    int res, profile, keyframe;
> >> +    int res, profile, keyframe, invisible, errorres;
> >>  
> >>      *out_data = data;
> >>      *out_size = size;
> >> @@ -42,21 +96,63 @@ static int parse(AVCodecParserContext *ctx,
> >>      profile  = get_bits1(&gb);
> >>      profile |= get_bits1(&gb) << 1;
> >>      if (profile == 3) profile += get_bits1(&gb);
> >> +    if (profile > 3)
> >> +        return size;
> >>  
> >>      if (get_bits1(&gb)) {
> >>          keyframe = 0;
> >> +        skip_bits(&gb, 3);
> >>      } else {
> >>          keyframe  = !get_bits1(&gb);
> >>      }
> >>  
> >> +    invisible = !get_bits1(&gb);
> >> +    errorres  = get_bits1(&gb);
> >> +
> >>      if (!keyframe) {
> >> +        int intraonly = invisible ? get_bits1(&gb) : 0;
> >> +        if (!errorres)
> >> +            skip_bits(&gb, 2);
> >> +        if (intraonly) {
> >> +            if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
> >> +                return size;
> >> +            avctx->profile = profile;
> >> +            if (profile >= 1) {
> >> +                if ((read_colorspace_details(ctx, avctx, &gb)) < 0)
> >> +                    return size;
> >> +            } else {
> >> +                ctx->format        = AV_PIX_FMT_YUV420P;
> >> +                avctx->colorspace  = AVCOL_SPC_BT470BG;
> >> +                avctx->color_range = AVCOL_RANGE_MPEG;
> >> +            }
> >> +            skip_bits(&gb, 8); // refreshrefmask
> >> +            ctx->width  = get_bits(&gb, 16) + 1;
> >> +            ctx->height = get_bits(&gb, 16) + 1;
> >> +            if (get_bits1(&gb)) // display size
> >> +                skip_bits(&gb, 32);
> >> +        }
> >> +
> >>          ctx->pict_type = AV_PICTURE_TYPE_P;
> >>          ctx->key_frame = 0;
> >>      } else {
> >> +        if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
> >> +            return size;
> >> +        avctx->profile = profile;
> >> +        if (read_colorspace_details(ctx, avctx, &gb) < 0)
> >> +            return size;
> >> +        ctx->width  = get_bits(&gb, 16) + 1;
> >> +        ctx->height = get_bits(&gb, 16) + 1;
> >> +        if (get_bits1(&gb)) // display size
> >> +            skip_bits(&gb, 32);
> >> +
> >>          ctx->pict_type = AV_PICTURE_TYPE_I;
> >>          ctx->key_frame = 1;
> >>      }
> >>  
> > 
> >> +    if (ctx->width && (!avctx->width || !avctx->height ||
> >> +                       !avctx->coded_width || !avctx->coded_height))
> >> +        ff_set_dimensions(avctx, ctx->width, ctx->height);
> > 
> > This is the only ff_set_dimensions() in the codebase with non fixed parameters
> > that ignores the return value 
> > is that intended ?
> 
> Yes, since right after it the only line that remains in the entire
> function is the return.
> 
> I can change it to "if (ff_set_dimensions() < 0) return size" if that's
> preferred, but it will be functionally the same since parsers can't
> return errors, so they always return the full frame size.

maybe write something like (void)ff_set_dimensions()
so its clear that you intended to ignore the return

[...]
James Almer Jan. 20, 2018, 1:04 a.m.
On 1/19/2018 9:36 PM, Michael Niedermayer wrote:
> On Fri, Jan 19, 2018 at 09:33:46PM -0300, James Almer wrote:
>> On 1/19/2018 8:56 PM, Michael Niedermayer wrote:
>>> On Fri, Jan 19, 2018 at 12:51:21AM -0300, James Almer wrote:
>>>> Improves remuxing support when vp9 decoder is not compiled in.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  libavcodec/vp9_parser.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 97 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
>>>> index 9531f34a32..b059477747 100644
>>>> --- a/libavcodec/vp9_parser.c
>>>> +++ b/libavcodec/vp9_parser.c
>>>> @@ -23,15 +23,69 @@
>>>>  
>>>>  #include "libavutil/intreadwrite.h"
>>>>  #include "libavcodec/get_bits.h"
>>>> +#include "libavcodec/internal.h"
>>>>  #include "parser.h"
>>>>  
>>>> +#define VP9_SYNCCODE 0x498342
>>>> +
>>>> +static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
>>>> +                                   GetBitContext *gb)
>>>> +{
>>>> +    static const enum AVColorSpace colorspaces[8] = {
>>>> +        AVCOL_SPC_UNSPECIFIED, AVCOL_SPC_BT470BG, AVCOL_SPC_BT709, AVCOL_SPC_SMPTE170M,
>>>> +        AVCOL_SPC_SMPTE240M, AVCOL_SPC_BT2020_NCL, AVCOL_SPC_RESERVED, AVCOL_SPC_RGB,
>>>> +    };
>>>> +    int bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb); // 0:8, 1:10, 2:12
>>>> +
>>>> +    avctx->colorspace = colorspaces[get_bits(gb, 3)];
>>>> +    if (avctx->colorspace == AVCOL_SPC_RGB) { // RGB = profile 1
>>>> +        static const enum AVPixelFormat pix_fmt_rgb[3] = {
>>>> +            AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12
>>>> +        };
>>>> +        if (avctx->profile & 1) {
>>>> +            if (get_bits1(gb)) // reserved bit
>>>> +                return AVERROR_INVALIDDATA;
>>>> +        } else
>>>> +            return AVERROR_INVALIDDATA;
>>>> +        avctx->color_range = AVCOL_RANGE_JPEG;
>>>> +        ctx->format = pix_fmt_rgb[bits];
>>>> +    } else {
>>>> +        static const enum AVPixelFormat pix_fmt_for_ss[3][2 /* v */][2 /* h */] = {
>>>> +            { { AV_PIX_FMT_YUV444P,   AV_PIX_FMT_YUV422P },
>>>> +              { AV_PIX_FMT_YUV440P,   AV_PIX_FMT_YUV420P } },
>>>> +            { { AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV422P10 },
>>>> +              { AV_PIX_FMT_YUV440P10, AV_PIX_FMT_YUV420P10 } },
>>>> +            { { AV_PIX_FMT_YUV444P12, AV_PIX_FMT_YUV422P12 },
>>>> +              { AV_PIX_FMT_YUV440P12, AV_PIX_FMT_YUV420P12 } }
>>>> +        };
>>>> +        avctx->color_range = get_bits1(gb) ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
>>>> +        if (avctx->profile & 1) {
>>>> +            int ss_h, ss_v, format;
>>>> +
>>>> +            ss_h = get_bits1(gb);
>>>> +            ss_v = get_bits1(gb);
>>>> +            format = pix_fmt_for_ss[bits][ss_v][ss_h];
>>>> +            if (format == AV_PIX_FMT_YUV420P)
>>>> +                // YUV 4:2:0 not supported in profiles 1 and 3
>>>> +                return AVERROR_INVALIDDATA;
>>>> +            else if (get_bits1(gb)) // color details reserved bit
>>>> +                return AVERROR_INVALIDDATA;
>>>> +            ctx->format = format;
>>>> +        } else {
>>>> +            ctx->format = pix_fmt_for_ss[bits][1][1];
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  static int parse(AVCodecParserContext *ctx,
>>>>                   AVCodecContext *avctx,
>>>>                   const uint8_t **out_data, int *out_size,
>>>>                   const uint8_t *data, int size)
>>>>  {
>>>>      GetBitContext gb;
>>>> -    int res, profile, keyframe;
>>>> +    int res, profile, keyframe, invisible, errorres;
>>>>  
>>>>      *out_data = data;
>>>>      *out_size = size;
>>>> @@ -42,21 +96,63 @@ static int parse(AVCodecParserContext *ctx,
>>>>      profile  = get_bits1(&gb);
>>>>      profile |= get_bits1(&gb) << 1;
>>>>      if (profile == 3) profile += get_bits1(&gb);
>>>> +    if (profile > 3)
>>>> +        return size;
>>>>  
>>>>      if (get_bits1(&gb)) {
>>>>          keyframe = 0;
>>>> +        skip_bits(&gb, 3);
>>>>      } else {
>>>>          keyframe  = !get_bits1(&gb);
>>>>      }
>>>>  
>>>> +    invisible = !get_bits1(&gb);
>>>> +    errorres  = get_bits1(&gb);
>>>> +
>>>>      if (!keyframe) {
>>>> +        int intraonly = invisible ? get_bits1(&gb) : 0;
>>>> +        if (!errorres)
>>>> +            skip_bits(&gb, 2);
>>>> +        if (intraonly) {
>>>> +            if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
>>>> +                return size;
>>>> +            avctx->profile = profile;
>>>> +            if (profile >= 1) {
>>>> +                if ((read_colorspace_details(ctx, avctx, &gb)) < 0)
>>>> +                    return size;
>>>> +            } else {
>>>> +                ctx->format        = AV_PIX_FMT_YUV420P;
>>>> +                avctx->colorspace  = AVCOL_SPC_BT470BG;
>>>> +                avctx->color_range = AVCOL_RANGE_MPEG;
>>>> +            }
>>>> +            skip_bits(&gb, 8); // refreshrefmask
>>>> +            ctx->width  = get_bits(&gb, 16) + 1;
>>>> +            ctx->height = get_bits(&gb, 16) + 1;
>>>> +            if (get_bits1(&gb)) // display size
>>>> +                skip_bits(&gb, 32);
>>>> +        }
>>>> +
>>>>          ctx->pict_type = AV_PICTURE_TYPE_P;
>>>>          ctx->key_frame = 0;
>>>>      } else {
>>>> +        if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
>>>> +            return size;
>>>> +        avctx->profile = profile;
>>>> +        if (read_colorspace_details(ctx, avctx, &gb) < 0)
>>>> +            return size;
>>>> +        ctx->width  = get_bits(&gb, 16) + 1;
>>>> +        ctx->height = get_bits(&gb, 16) + 1;
>>>> +        if (get_bits1(&gb)) // display size
>>>> +            skip_bits(&gb, 32);
>>>> +
>>>>          ctx->pict_type = AV_PICTURE_TYPE_I;
>>>>          ctx->key_frame = 1;
>>>>      }
>>>>  
>>>
>>>> +    if (ctx->width && (!avctx->width || !avctx->height ||
>>>> +                       !avctx->coded_width || !avctx->coded_height))
>>>> +        ff_set_dimensions(avctx, ctx->width, ctx->height);
>>>
>>> This is the only ff_set_dimensions() in the codebase with non fixed parameters
>>> that ignores the return value 
>>> is that intended ?
>>
>> Yes, since right after it the only line that remains in the entire
>> function is the return.
>>
>> I can change it to "if (ff_set_dimensions() < 0) return size" if that's
>> preferred, but it will be functionally the same since parsers can't
>> return errors, so they always return the full frame size.
> 
> maybe write something like (void)ff_set_dimensions()
> so its clear that you intended to ignore the return

I find that kinda ugly, so amended locally to add a check for the result.

Patch hide | download patch | download mbox

diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
index 9531f34a32..b059477747 100644
--- a/libavcodec/vp9_parser.c
+++ b/libavcodec/vp9_parser.c
@@ -23,15 +23,69 @@ 
 
 #include "libavutil/intreadwrite.h"
 #include "libavcodec/get_bits.h"
+#include "libavcodec/internal.h"
 #include "parser.h"
 
+#define VP9_SYNCCODE 0x498342
+
+static int read_colorspace_details(AVCodecParserContext *ctx, AVCodecContext *avctx,
+                                   GetBitContext *gb)
+{
+    static const enum AVColorSpace colorspaces[8] = {
+        AVCOL_SPC_UNSPECIFIED, AVCOL_SPC_BT470BG, AVCOL_SPC_BT709, AVCOL_SPC_SMPTE170M,
+        AVCOL_SPC_SMPTE240M, AVCOL_SPC_BT2020_NCL, AVCOL_SPC_RESERVED, AVCOL_SPC_RGB,
+    };
+    int bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb); // 0:8, 1:10, 2:12
+
+    avctx->colorspace = colorspaces[get_bits(gb, 3)];
+    if (avctx->colorspace == AVCOL_SPC_RGB) { // RGB = profile 1
+        static const enum AVPixelFormat pix_fmt_rgb[3] = {
+            AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12
+        };
+        if (avctx->profile & 1) {
+            if (get_bits1(gb)) // reserved bit
+                return AVERROR_INVALIDDATA;
+        } else
+            return AVERROR_INVALIDDATA;
+        avctx->color_range = AVCOL_RANGE_JPEG;
+        ctx->format = pix_fmt_rgb[bits];
+    } else {
+        static const enum AVPixelFormat pix_fmt_for_ss[3][2 /* v */][2 /* h */] = {
+            { { AV_PIX_FMT_YUV444P,   AV_PIX_FMT_YUV422P },
+              { AV_PIX_FMT_YUV440P,   AV_PIX_FMT_YUV420P } },
+            { { AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV422P10 },
+              { AV_PIX_FMT_YUV440P10, AV_PIX_FMT_YUV420P10 } },
+            { { AV_PIX_FMT_YUV444P12, AV_PIX_FMT_YUV422P12 },
+              { AV_PIX_FMT_YUV440P12, AV_PIX_FMT_YUV420P12 } }
+        };
+        avctx->color_range = get_bits1(gb) ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
+        if (avctx->profile & 1) {
+            int ss_h, ss_v, format;
+
+            ss_h = get_bits1(gb);
+            ss_v = get_bits1(gb);
+            format = pix_fmt_for_ss[bits][ss_v][ss_h];
+            if (format == AV_PIX_FMT_YUV420P)
+                // YUV 4:2:0 not supported in profiles 1 and 3
+                return AVERROR_INVALIDDATA;
+            else if (get_bits1(gb)) // color details reserved bit
+                return AVERROR_INVALIDDATA;
+            ctx->format = format;
+        } else {
+            ctx->format = pix_fmt_for_ss[bits][1][1];
+        }
+    }
+
+    return 0;
+}
+
 static int parse(AVCodecParserContext *ctx,
                  AVCodecContext *avctx,
                  const uint8_t **out_data, int *out_size,
                  const uint8_t *data, int size)
 {
     GetBitContext gb;
-    int res, profile, keyframe;
+    int res, profile, keyframe, invisible, errorres;
 
     *out_data = data;
     *out_size = size;
@@ -42,21 +96,63 @@  static int parse(AVCodecParserContext *ctx,
     profile  = get_bits1(&gb);
     profile |= get_bits1(&gb) << 1;
     if (profile == 3) profile += get_bits1(&gb);
+    if (profile > 3)
+        return size;
 
     if (get_bits1(&gb)) {
         keyframe = 0;
+        skip_bits(&gb, 3);
     } else {
         keyframe  = !get_bits1(&gb);
     }
 
+    invisible = !get_bits1(&gb);
+    errorres  = get_bits1(&gb);
+
     if (!keyframe) {
+        int intraonly = invisible ? get_bits1(&gb) : 0;
+        if (!errorres)
+            skip_bits(&gb, 2);
+        if (intraonly) {
+            if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
+                return size;
+            avctx->profile = profile;
+            if (profile >= 1) {
+                if ((read_colorspace_details(ctx, avctx, &gb)) < 0)
+                    return size;
+            } else {
+                ctx->format        = AV_PIX_FMT_YUV420P;
+                avctx->colorspace  = AVCOL_SPC_BT470BG;
+                avctx->color_range = AVCOL_RANGE_MPEG;
+            }
+            skip_bits(&gb, 8); // refreshrefmask
+            ctx->width  = get_bits(&gb, 16) + 1;
+            ctx->height = get_bits(&gb, 16) + 1;
+            if (get_bits1(&gb)) // display size
+                skip_bits(&gb, 32);
+        }
+
         ctx->pict_type = AV_PICTURE_TYPE_P;
         ctx->key_frame = 0;
     } else {
+        if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
+            return size;
+        avctx->profile = profile;
+        if (read_colorspace_details(ctx, avctx, &gb) < 0)
+            return size;
+        ctx->width  = get_bits(&gb, 16) + 1;
+        ctx->height = get_bits(&gb, 16) + 1;
+        if (get_bits1(&gb)) // display size
+            skip_bits(&gb, 32);
+
         ctx->pict_type = AV_PICTURE_TYPE_I;
         ctx->key_frame = 1;
     }
 
+    if (ctx->width && (!avctx->width || !avctx->height ||
+                       !avctx->coded_width || !avctx->coded_height))
+        ff_set_dimensions(avctx, ctx->width, ctx->height);
+
     return size;
 }