Re: [PATCH v2 1/2] media: uapi: Add H264 low-level decoder API compound controls.

[Maxime Ripard <maxime.ripard@bootlin.com>]

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

On Thu, Jan 24, 2019 at 10:37:23PM +0800, Ayaka wrote:
> >>>>> +#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
> >>>>> +
> >>>>> +struct v4l2_h264_dpb_entry {
> >>>>> +    __u32 tag;
> >>>>> +    __u16 frame_num;
> >>>>> +    __u16 pic_num;
> >>>> Although the long term reference would use picture order count
> >>>> and short term for frame num, but only one of them is used
> >>>> for a entry of a dpb.
> >>>> 
> >>>> Besides, for a frame picture frame_num = pic_num * 2,
> >>>> and frame_num = pic_num * 2 + 1 for a filed.
> >>> 
> >>> I'm not sure what is your point?
> >> 
> >> I found I was wrong at the last email.
> >> 
> >> But stateless hardware decoder usually don't care about whether it is long
> >> term or short term, as the real dpb updating or management work are not done
> >> by the the driver or device and decoding job would only use the two list(or
> >> one list for slice P) for reference pictures. So those flag for long term or
> >> status can be removed as well.
> > 
> > I'll remove the LONG_TERM flag then. We do need the other two for the
> > Allwinner driver though.
> >
>
> I would ask Paulk and check the manual and vendor library later.
>
> Even there are two register fields, it don’t mean they would be used
> and required at the same times. Because it don’t follow ISO manual.

It's not a matter of decoding per se, but how the hardware
behaves. All the buffers needed for one particular frame to be decoded
are uploaded to an SRAM, and the position of each buffer in that SRAM
cannot change during the time when it has been decoded, and then later
on when it's used as a reference. If you only have the frames needed
to decode the current frame, you will have no idea which slot in the
SRAM can be reused, whereas having the full DPB allows you to do
that. And that's what _FLAG_ACTIVE gives you.

> >> And I agree above with my last mail, so I would suggest to keep a property
> >> as index for both frame_num and pic_num, as only one of them would be used
> >> for a picture decoding once time.
> > 
> > I'd really prefer to keep everything that is in the bitstream defined
> > here. We don't want to cover the usual cases, but all of them even the
> > one that haven't been designed yet, so we should be really
> > conservative.
>
> As I mention in the other mail, a stateless decoder or encoder like
> means the device won’t track the previous result. But you have no
> idea on what data the device would need to process this picture. It
> is hard to define a standard structure for it.
>
> As you see, even allwinner doesn’t obey all the standard the IOS
> document said.

It's not that it disobeys it, it's that it requires the full blown DPB
to have a working driver.

> In my original suggestion, I would just to add more reservation
> fields then future driver can use it.

This interface is not stable at the moment, so it doesn't really
matter does it?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]