[FFmpeg-devel] [PATCH] avcodec/vp9_parser: export profile and pixel format
Chris Cunningham
chcunningham at chromium.org
Tue Oct 30 00:33:49 EET 2018
Thanks for pushing the profile part. Turns out I'm also interested in pixel
format, but I follow your comments about dropping the rest of patch. With
the code as-is, how busted are we when parsing a super frame. Does it
correctly grab the profile of the first frame?
Chrome actually has a fairly complete VP9 parser
<https://cs.chromium.org/chromium/src/media/filters/vp9_parser.cc?rcl=adf255ef1da95255174643a6c1482e4305f877fd&l=478>
but the CodecPrivate is no longer set in extradata for VP9 so we don't
(AFAIK) have easy access to the frame header. Would you be open to
surfacing codec private via extradata (reverting this change
<http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225548.html>)?
On Thu, Oct 25, 2018 at 4:50 PM James Almer <jamrial at gmail.com> wrote:
> On 10/24/2018 3:12 PM, Chris Cunningham wrote:
> > On Wed, Oct 3, 2018 at 10:49 AM James Almer <jamrial at gmail.com> wrote:
> >
> >> Signed-off-by: James Almer <jamrial at gmail.com>
> >> ---
> >> libavcodec/vp9_parser.c | 82 ++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 80 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> >> index 9531f34a32..d4110f20bf 100644
> >> --- a/libavcodec/vp9_parser.c
> >> +++ b/libavcodec/vp9_parser.c
> >> @@ -23,36 +23,114 @@
> >>
> >> #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 colorspace, bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb);
> >> // 0:8, 1:10, 2:12
> >> +
> >> + colorspace = colorspaces[get_bits(gb, 3)];
> >> + if (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;
> >> + 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 } }
> >> + };
> >> + 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;
> >>
> >> - if ((res = init_get_bits8(&gb, data, size)) < 0)
> >> + if (!size || (res = init_get_bits8(&gb, data, size)) < 0)
> >> return size; // parsers can't return errors
> >> get_bits(&gb, 2); // frame marker
> >> profile = get_bits1(&gb);
> >> profile |= get_bits1(&gb) << 1;
> >> if (profile == 3) profile += get_bits1(&gb);
> >> + if (profile > 3)
> >> + return size;
> >>
> >> + avctx->profile = profile;
> >> 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;
> >> + if (profile >= 1) {
> >> + if ((read_colorspace_details(ctx, avctx, &gb)) < 0)
> >> + return size;
> >> + } else {
> >> + ctx->format = AV_PIX_FMT_YUV420P;
> >> + }
> >> + }
> >> +
> >> ctx->pict_type = AV_PICTURE_TYPE_P;
> >> ctx->key_frame = 0;
> >> } else {
> >> + if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
> >> + return size;
> >> + if (read_colorspace_details(ctx, avctx, &gb) < 0)
> >> + return size;
> >> +
> >> ctx->pict_type = AV_PICTURE_TYPE_I;
> >> ctx->key_frame = 1;
> >> }
> >> --
> >> 2.19.0
> >>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel at ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >
> > Friendly ping for review.
>
> Dropping this as it's not correct. It's not taking into account
> show_existing_frame frames (And reading bits way past the header bits),
> or superframes, where it may be parsing an invisible frame.
>
> Pushed only the part setting profile.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list