Re: [RFC 07/12] media: uapi: h264: Add DPB entry field reference flags

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 8063 bytes --]

Le samedi 11 juillet 2020 à 10:21 +0000, Jonas Karlman a écrit :
> On 2020-07-10 23:49, Nicolas Dufresne wrote:
> > Le vendredi 10 juillet 2020 à 09:25 -0300, Ezequiel Garcia a écrit :
> > > +Nicolas
> > > 
> > > On Fri, 2020-07-10 at 14:05 +0200, Boris Brezillon wrote:
> > > > On Fri, 10 Jul 2020 08:50:28 -0300
> > > > Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > 
> > > > > On Fri, 2020-07-10 at 10:13 +0200, Boris Brezillon wrote:
> > > > > > On Fri, 10 Jul 2020 01:21:07 -0300
> > > > > > Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > >   
> > > > > > > Hello Jonas,
> > > > > > > 
> > > > > > > In the context of the uAPI cleanup,
> > > > > > > I'm revisiting this patch.
> > > > > > > 
> > > > > > > On Sun, 2019-09-01 at 12:45 +0000, Jonas Karlman wrote:  
> > > > > > > > Add DPB entry flags to help indicate when a reference frame is a
> > > > > > > > field picture
> > > > > > > > and how the DPB entry is referenced, top or bottom field or full
> > > > > > > > frame.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > > > > > ---
> > > > > > > >  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++++++
> > > > > > > >  include/media/h264-ctrls.h                       |  4 ++++
> > > > > > > >  2 files changed, 16 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > > > b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > > > index bc5dd8e76567..eb6c32668ad7 100644
> > > > > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > > > @@ -2022,6 +2022,18 @@ enum
> > > > > > > > v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > > > > >      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
> > > > > > > >        - 0x00000004
> > > > > > > >        - The DPB entry is a long term reference frame
> > > > > > > > +    * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE``
> > > > > > > > +      - 0x00000008
> > > > > > > > +      - The DPB entry is a field picture
> > > > > > > > +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_TOP``
> > > > > > > > +      - 0x00000010
> > > > > > > > +      - The DPB entry is a top field reference
> > > > > > > > +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM``
> > > > > > > > +      - 0x00000020
> > > > > > > > +      - The DPB entry is a bottom field reference
> > > > > > > > +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME``
> > > > > > > > +      - 0x00000030
> > > > > > > > +      - The DPB entry is a reference frame
> > > > > > > >  
> > > > > > > >  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
> > > > > > > >      Specifies the decoding mode to use. Currently exposes slice-
> > > > > > > > based and
> > > > > > > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > > > > > > > index e877bf1d537c..76020ebd1e6c 100644
> > > > > > > > --- a/include/media/h264-ctrls.h
> > > > > > > > +++ b/include/media/h264-ctrls.h
> > > > > > > > @@ -185,6 +185,10 @@ struct v4l2_ctrl_h264_slice_params {
> > > > > > > >  #define V4L2_H264_DPB_ENTRY_FLAG_VALID		0x01
> > > > > > > >  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
> > > > > > > >  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
> > > > > > > > +#define V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE	0x08
> > > > > > > > +#define V4L2_H264_DPB_ENTRY_FLAG_REF_TOP	0x10
> > > > > > > > +#define V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM	0x20
> > > > > > > > +#define V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME	0x30
> > > > > > > >      
> > > > > > > 
> > > > > > > I've been going thru the H264 spec and I'm unsure,
> > > > > > > are all these flags semantically needed?
> > > > > > > 
> > > > > > > For instance, if one of REF_BOTTOM or REF_TOP (or both)
> > > > > > > are set, doesn't that indicate it's a field picture?
> > > > > > > 
> > > > > > > Or conversely, if neither REF_BOTTOM or REF_TOP are set,
> > > > > > > then it's a frame picture?  
> > > > > > 
> > > > > > I think that's what I was trying to do here [1]
> > > > > > 
> > > > > > [1]https://patchwork.kernel.org/patch/11392095/  
> > > > > 
> > > > > Right. Aren't we missing a DPB_ENTRY_FLAG_TOP_FIELD?
> > > > > 
> > > > > If I understand correctly, the DPB can contain:
> > > > > 
> > > > > * frames (FLAG_FIELD not set)
> > > > > * a field pair, with a single field (FLAG_FIELD and either TOP or BOTTOM).
> > > > > * a field pair, with boths fields (FLAG_FIELD and both TOP or BOTTOM).
> > > > 
> > > > Well, my understand is that, if the buffer contains both a TOP and
> > > > BOTTOM field, it actually becomes a full frame, so you actually have
> > > > those cases:
> > > > 
> > > > * FLAG_FIELD not set: this a frame (note that a TOP/BOTTOM field
> > > >   decoded buffer can become of frame if it's complemented with the
> > > >   missing field later during the decoding)
> > > > * FLAG_FIELD set + BOTTOM_FIELD not set: this is a TOP field
> > > > * FLAG_FIELD set + BOTTOM_FIELD set: this is a BOTTOM field
> > > > * FLAG_FIELD not set + BOTTOM_FIELD set: invalid combination
> > 
> > Let's admit, while this work, it's odd. Can we just move to that instewad ?
> > 
> >   FLAG_TOP_FIELD
> >   FLAG_BOTTOM_FIELD
> >   FLAG_FRAME = (FLAG_TOP_FIELD | FLAG_BOTTOM_FIELD)
> > 
> > So it can be used as a flag, but also is a proper enum and there is no longer an
> > invalid combination.
> >   
> > > > but I might be wrong.
> 
> There seems to be some misunderstanding here, the top/bottom flagging should
> not be used to describe if the picture is a field, field pair or frame, it
> should be used to flag if a frame or the top and/or bottom field (in case of
> a field pair) is "used for short-term reference".
> 
> FLAG_TOP_REF
> FLAG_BOTTOM_REF
> FLAG_FRAME_REF = (FLAG_TOP_REF | FLAG_BOTTOM_REF)
> 
> Would be a more appropriate naming.

It's a subtle nuance, but could work.

The reason I referred to it like this is because in gstreamer-vaapi,
this information is deduced from picture->structure flags (I believe
it's inspired from JM reference decoder). This structure is updated
when a specific field has been decoded. So it effectively represent
which field of that picture are valid/decoded, and the combination of
this picture being reference and that flag is the only state used to
communicate that information. The real use for this is for the case we
have lost a field. A missing reference picture can then be detected.

So in gstreamer-vaapi, the case where you have both top/bottom field of
a reference being decoded, but only one of the field marked for
reference in the DPB does not exist. I don't know if that really exist
in H.264.

> 
> The FIELD_PIC flag would then be used to describe if the picture is a
> reference frame or a complementary reference field pair.
> 
> As described in hantro h264 driver [1] the MV buffer is split in two
> for field encoded frames, and I guess the rkvdec block does something
> similar and therefore the HW blocks probably needs to know if the reference
> picture is a reference frame or a complementary reference field pair.
> It should be possible to keep such state in driver but since such information
> was easily available in ffmpeg and the driver being "stateless" using a flag
> seamed like a good choice at the time.
> 
> Please note that I have not done any test without the "field pic" flagging
> but both mpp and the imx/hantro reference code are configuring this bit.
> 
> [1] https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/hantro/hantro_g1_h264_dec.c#n265
> 
> Regards,
> Jonas
> 
> > > Yes, perhaps that's correct. I was trying to think strictly
> > > in terms of the H264 semantics, to define a clean interface.
> > > 
> > > From the mpp code, looks like the above is enough for rkvdec
> > > (although I haven't done any tests).
> > > 
> > > Ezequiel
> > > 
> > > 
> > > 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]