Hi,
Apologies if you've covered any of these comments before.
On 03/10/2018 02:44, James Almer wrote:
> Simple parser to set keyframes, frame type, structure, width, height, and
> pixel
> format, plus stream profile and level.
>
> Signed-off-by: James Almer <jamr...@gmail.com>
> ---
> Missing Changelog entry and version bump still.
[...]
> + if (avctx->extradata_size && !s->parsed_extradata) {
> + s->parsed_extradata = 1;
Shouldn't this be set at the bottom of this block (since parsing can fail)?
> + }
> +
> + avctx->profile = seq->seq_profile;
> + avctx->level = seq->seq_level_idx[0];
> +
> + switch (frame_type) {
> + case AV1_FRAME_KEY:
> + case AV1_FRAME_INTRA_ONLY:
> + ctx->pict_type = AV_PICTURE_TYPE_I;
> + break;
Not strictly related to this patch, and not a blocker, but is it not
about time the API gains the ability to mark frames with more clarity
than just "I"? H.264, HEVC, etc. also have this issue (e.g. I vs IDR,
and HEVC's many times of RAPs). AV_PICTURE_TYPE_SI is kinda related,
I guess, but not exactly what I mean.
> + case AV1_FRAME_INTER:
> + ctx->pict_type = AV_PICTURE_TYPE_P;
> + break;
> + case AV1_FRAME_SWITCH:
> + ctx->pict_type = AV_PICTURE_TYPE_SP;
> + break;
> + }
I assume we're not going to mark alt-refs in any special way since
they're combined in one packet with a visible frame?
> + ctx->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
Any reason we just don't always set this at the top of the function?
> + switch (av1->bit_depth) {
> + case 8:
> + ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY8
> + : pix_fmts_8bit
> [color->subsampling_x][color->subsampling_y];
> + break;
> + case 10:
> + ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY10
> + :
> pix_fmts_10bit[color->subsampling_x][color->subsampling_y];
> + break;
> + case 12:
> + ctx->format = color->mono_chrome ? AV_PIX_FMT_GRAY12
> + :
> pix_fmts_12bit[color->subsampling_x][color->subsampling_y];
> + break;
> + }
Won't this break horribly on e.g. 4:4:0?
> + av_assert2(ctx->format != AV_PIX_FMT_NONE);
I assume ctx->bit_depth will always be set to something valid.
- Derek
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
