On Wed, Jul 15, 2020 at 4:13 PM Mark Thompson <s...@jkqxz.net> wrote:
> On 15/07/2020 18:43, Yongle Lin wrote:
> > add block type field to AVVideoBlockParams so we could either export or
> visualize it later.
> > ---
> > libavutil/video_enc_params.h | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h
> > index 43fa443154..8bf5f240c9 100644
> > --- a/libavutil/video_enc_params.h
> > +++ b/libavutil/video_enc_params.h
> > @@ -57,6 +57,11 @@ enum AVVideoEncParamsType {
> > AV_VIDEO_ENC_PARAMS_H264,
> > };
> >
> > +enum AVVideoBlockFlags {
> > + AV_VIDEO_ENC_BLOCK_INTRA = 1ULL << 0, /* Indicates block uses
> intra prediction */
>
> The ULL is only confusing matters here - standard-conforming enum values
> have type int. Some compilers allow it to be a larger integer type, but I
> don't think we can rely on that extension.
>
I am thinking of defining a bit field struct to for type flags like what we
did in other places:
struct AVVideoBlockFlags {
unsigned int intra:1;
unsigned int skip:1;
}
Or we could use the same way of mb_type defined in H.264 like
#define AV_VIDEO_ENC_BLOCK_INTRA 1ULL << 0
#define AV_VIDEO_ENC_BLOCK_SKIP 1ULL << 1
uint64_t b_type
> > + AV_VIDEO_ENC_BLOCK_SKIP = 1ULL << 1, /* Indicates block is not
> coded (skipped) */
>
> Can you define more precisely what you mean by "not coded"? Is that just
> that it has no residuals, or does it also indicate no motion vector, or no
> coded motion vector relative to prediction?
>
I want to make it more general which can be applied to more codec. So in
VP9, there is a skip flag to indicate if the block has residual
coefficients. As for H.264 you mentioned there are P_Skip and B_Skip, I
think both of them should be considered as skip.
>
> > +};
> > +
> > /**
> > * Video encoding parameters for a given frame. This struct is
> allocated along
> > * with an optional array of per-block AVVideoBlockParams descriptors.
> > @@ -126,6 +131,20 @@ typedef struct AVVideoBlockParams {
> > * corresponding per-frame value.
> > */
> > int32_t delta_qp;
> > +
> > + /**
> > + * Type flag of the block
> > + * Each bit field indicates a type flag
> > + */
> > + enum AVVideoBlockFlags flags;
>
> Don't make this an enum, since you aren't using it as an enum - you're
> going to assign combinations of flags. (Also, the size of the field may
> change as more flags are added.)
>
> > +
> > + /**
> > + * Reference frames used for prediction
> > + * Each entry specifies the first/second/third/etc. reference frame
> the current frame uses.
> > + * The value at each entry specifies the index inside the reference
> frame array for that current frame.
>
> You'll need to define more carefully what "the reference frame array"
> means. I can guess that it's the ref_frame_idx[] number for VP9, but it's
> not at all obvious what it would mean for H.264.
>
Previously I planned to store if the block uses previous or future ref
frames for this field. I don't fully understand how H,264 stores the
reference frame. Perhaps we could change the definition of ref so that it
can be applied to both vp9 and h264.
> > + * Any entry that is unused will be set to -1
> > + */
> > + int8_t ref[8];
> > } AVVideoBlockParams;
> >
> > /*
> >
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Best,
Yongle
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
