| Message ID | 20180119035121.4612-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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 ? [...]
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.
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 [...]
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.
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; }
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(-)