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

James Almer jamrial at gmail.com
Sat Jan 20 03:04:50 EET 2018


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 at 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.


More information about the ffmpeg-devel mailing list