| Message ID | 20200706210817.2502907-1-yonglel@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] libavutil/video_enc_params: add block type | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 06/07/2020 22:08, Yongle Lin wrote: > add block type field to AVVideoBlockParams so we could either export or visualize it later. > --- > libavutil/video_enc_params.h | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h > index 43fa443154..55b9fc4031 100644 > --- a/libavutil/video_enc_params.h > +++ b/libavutil/video_enc_params.h > @@ -101,6 +101,21 @@ typedef struct AVVideoEncParams { > int32_t delta_qp[4][2]; > } AVVideoEncParams; > > +typedef struct MacroBlockType { > + /** > + * Is intra prediction > + */ > + int intra; > + /** > + * Skip flag > + */ > + int skip; > + /** > + * Reference to the past or future > + */ > + int ref[2]; Please can you define carefully in the documentation exactly what each of these fields mean, as is done for the QP values above? (That is, there should be enough information to determine what exactly is meant if I am given one of these structures with, say, intra = 3, skip = 7, ref = { 5, 1 }.) > +} MacroBlockType; Structures in the public API need to carry the "AV" namespace prefix. I'm not sure that "macroblock" is a good word to use here: many codecs have no concept called a "macroblock", and invoking a word with a specific definition in only some contexts seems unhelpful. > + > /** > * Data structure for storing block-level encoding information. > * It is allocated as a part of AVVideoEncParams and should be retrieved with > @@ -126,6 +141,11 @@ typedef struct AVVideoBlockParams { > * corresponding per-frame value. > */ > int32_t delta_qp; > + > + /** > + * Type of block > + */ > + MacroBlockType mb_type; > } AVVideoBlockParams; > > /* > - Mark
Quoting Yongle Lin (2020-07-06 23:08:17) > add block type field to AVVideoBlockParams so we could either export or visualize it later. > --- > libavutil/video_enc_params.h | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) We generally require new APIs to be immediately useful. So in this case, there should also be a patch that makes some decoder export those fields. > > diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h > index 43fa443154..55b9fc4031 100644 > --- a/libavutil/video_enc_params.h > +++ b/libavutil/video_enc_params.h > @@ -101,6 +101,21 @@ typedef struct AVVideoEncParams { > int32_t delta_qp[4][2]; > } AVVideoEncParams; > > +typedef struct MacroBlockType { > + /** > + * Is intra prediction > + */ > + int intra; > + /** > + * Skip flag > + */ > + int skip; These structures are stored per-block, so it seems pretty wasteful to spend a whole int (4 bytes) when only one bit is used.
On Mon, Jul 6, 2020 at 3:08 PM Mark Thompson <sw@jkqxz.net> wrote: > On 06/07/2020 22:08, Yongle Lin wrote: > > add block type field to AVVideoBlockParams so we could either export or > visualize it later. > > --- > > libavutil/video_enc_params.h | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h > > index 43fa443154..55b9fc4031 100644 > > --- a/libavutil/video_enc_params.h > > +++ b/libavutil/video_enc_params.h > > @@ -101,6 +101,21 @@ typedef struct AVVideoEncParams { > > int32_t delta_qp[4][2]; > > } AVVideoEncParams; > > > > +typedef struct MacroBlockType { > > + /** > > + * Is intra prediction > > + */ > > + int intra; > > + /** > > + * Skip flag > > + */ > > + int skip; > > + /** > > + * Reference to the past or future > > + */ > > + int ref[2]; > > Please can you define carefully in the documentation exactly what each of > these fields mean, as is done for the QP values above? > > (That is, there should be enough information to determine what exactly is > meant if I am given one of these structures with, say, intra = 3, skip = 7, > ref = { 5, 1 }.) > I will add more detailed explanation to the comment. These fields are all boolean and I will add bit fields to minimize the size. > > > +} MacroBlockType; > > Structures in the public API need to carry the "AV" namespace prefix. > > I'm not sure that "macroblock" is a good word to use here: many codecs > have no concept called a "macroblock", and invoking a word with a specific > definition in only some contexts seems unhelpful. > I will change the name to make it more meaningful. > > > + > > /** > > * Data structure for storing block-level encoding information. > > * It is allocated as a part of AVVideoEncParams and should be > retrieved with > > @@ -126,6 +141,11 @@ typedef struct AVVideoBlockParams { > > * corresponding per-frame value. > > */ > > int32_t delta_qp; > > + > > + /** > > + * Type of block > > + */ > > + MacroBlockType mb_type; > > } 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-request@ffmpeg.org with subject "unsubscribe".
On Tue, Jul 7, 2020 at 12:59 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Yongle Lin (2020-07-06 23:08:17) > > add block type field to AVVideoBlockParams so we could either export or > visualize it later. > > --- > > libavutil/video_enc_params.h | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > We generally require new APIs to be immediately useful. So in this case, > there should also be a patch that makes some decoder export those > fields. > > > > > diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h > > index 43fa443154..55b9fc4031 100644 > > --- a/libavutil/video_enc_params.h > > +++ b/libavutil/video_enc_params.h > > @@ -101,6 +101,21 @@ typedef struct AVVideoEncParams { > > int32_t delta_qp[4][2]; > > } AVVideoEncParams; > > > > +typedef struct MacroBlockType { > > + /** > > + * Is intra prediction > > + */ > > + int intra; > > + /** > > + * Skip flag > > + */ > > + int skip; > > These structures are stored per-block, so it seems pretty wasteful to > spend a whole int (4 bytes) when only one bit is used. > You are right. I should add bit fields to restrict the size. Do you think it makes more sense to unwrap the fields and put them all in AVVideoBlockParams or pack them in a struct like this? Thanks. > > -- > Anton Khirnov > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jul 7, 2020, 19:22 by yonglel-at-google.com@ffmpeg.org: > On Tue, Jul 7, 2020 at 12:59 AM Anton Khirnov <anton@khirnov.net> wrote: > >> Quoting Yongle Lin (2020-07-06 23:08:17) >> > add block type field to AVVideoBlockParams so we could either export or >> visualize it later. >> > --- >> > libavutil/video_enc_params.h | 20 ++++++++++++++++++++ >> > 1 file changed, 20 insertions(+) >> >> We generally require new APIs to be immediately useful. So in this case, >> there should also be a patch that makes some decoder export those >> fields. >> >> > >> > diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h >> > index 43fa443154..55b9fc4031 100644 >> > --- a/libavutil/video_enc_params.h >> > +++ b/libavutil/video_enc_params.h >> > @@ -101,6 +101,21 @@ typedef struct AVVideoEncParams { >> > int32_t delta_qp[4][2]; >> > } AVVideoEncParams; >> > >> > +typedef struct MacroBlockType { >> > + /** >> > + * Is intra prediction >> > + */ >> > + int intra; >> > + /** >> > + * Skip flag >> > + */ >> > + int skip; >> >> These structures are stored per-block, so it seems pretty wasteful to >> spend a whole int (4 bytes) when only one bit is used. >> > You are right. I should add bit fields to restrict the size. Do you think > it makes more sense to unwrap the fields and put them all in > AVVideoBlockParams or pack them in a struct like this? Thanks. > Put them in the AVVideoBlockParams struct. It was designed to be extended in that way and currently has only 5 entries. I'd suggest making it a uint64_t bitfield for all booleans that we might have to use in the future. As for the references field, I'd suggest making that at least 8 entries. We don't know what the future would look like so even if its somewhat wasteful now, we'll save ourselves a lot of trouble later if blocks may have more than 2 references.
On Tue, Jul 7, 2020 at 12:59 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Yongle Lin (2020-07-06 23:08:17) > > add block type field to AVVideoBlockParams so we could either export or > visualize it later. > > --- > > libavutil/video_enc_params.h | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > We generally require new APIs to be immediately useful. So in this case, > there should also be a patch that makes some decoder export those > fields. > I plan to send separate patches for H264 and VP9 to export type data and another patch to visualize the block type in codecview filter when I get approved for this patch. Do you want me to add the "decoder export" code to this patch so that it will be immediately useful? Thanks > > > > diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h > > index 43fa443154..55b9fc4031 100644 > > --- a/libavutil/video_enc_params.h > > +++ b/libavutil/video_enc_params.h > > @@ -101,6 +101,21 @@ typedef struct AVVideoEncParams { > > int32_t delta_qp[4][2]; > > } AVVideoEncParams; > > > > +typedef struct MacroBlockType { > > + /** > > + * Is intra prediction > > + */ > > + int intra; > > + /** > > + * Skip flag > > + */ > > + int skip; > > These structures are stored per-block, so it seems pretty wasteful to > spend a whole int (4 bytes) when only one bit is used. > > -- > Anton Khirnov > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavutil/video_enc_params.h b/libavutil/video_enc_params.h index 43fa443154..55b9fc4031 100644 --- a/libavutil/video_enc_params.h +++ b/libavutil/video_enc_params.h @@ -101,6 +101,21 @@ typedef struct AVVideoEncParams { int32_t delta_qp[4][2]; } AVVideoEncParams; +typedef struct MacroBlockType { + /** + * Is intra prediction + */ + int intra; + /** + * Skip flag + */ + int skip; + /** + * Reference to the past or future + */ + int ref[2]; +} MacroBlockType; + /** * Data structure for storing block-level encoding information. * It is allocated as a part of AVVideoEncParams and should be retrieved with @@ -126,6 +141,11 @@ typedef struct AVVideoBlockParams { * corresponding per-frame value. */ int32_t delta_qp; + + /** + * Type of block + */ + MacroBlockType mb_type; } AVVideoBlockParams; /*