Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Mon, 20 Jul 2020 20:03:38 +0530
From:   dikshita@codeaurora.org
To:     Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc:     Nicolas Dufresne <nicolas@ndufresne.ca>, mchehab@kernel.org,
        ezequiel@collabora.com, boris.brezillon@collabora.com,
        ribalda@kernel.org, paul.kocialkowski@bootlin.com,
        posciak@chromium.org, linux-media@vger.kernel.org,
        stanimir.varbanov@linaro.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, vgarodia@codeaurora.org,
        majja@codeaurora.org, linux-media-owner@vger.kernel.org
Subject: Re: [RFC PATCH 0/1] Add LTR controls
In-Reply-To: <d37e4e83-b7ae-7d44-c75f-2055f11ae898@xs4all.nl>
References: <1591871121-25420-1-git-send-email-dikshita@codeaurora.org>
 <f07c4aab69d2b333c0e36c50c526c0a85322e708.camel@ndufresne.ca>
 <1a9904b6-60a5-0faa-8a5e-c9dc00802184@xs4all.nl>
 <40040141fc3027c3eb1fdebc1a0e8ade@codeaurora.org>
 <d37e4e83-b7ae-7d44-c75f-2055f11ae898@xs4all.nl>
Message-ID: <dd365fe82523c4e44af4353f4f457137@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-07-16 14:25, Hans Verkuil wrote:
> On 16/06/2020 20:41, dikshita@codeaurora.org wrote:
>> Hi Hans, Nicolas,
>> 
>> Thanks for your comments.
>> 
>> On 2020-06-12 14:41, Hans Verkuil wrote:
>>> Hi Dikshita, Nicolas,
>>> 
>>> On 11/06/2020 16:22, Nicolas Dufresne wrote:
>>>> Le jeudi 11 juin 2020 à 15:55 +0530, Dikshita Agarwal a écrit :
>>>>> LTR (Long Term Reference) frames are the frames that are encoded
>>>>> sometime in the past
>>>>> and stored in the DPB buffer list to be used as reference to encode
>>>>> future frames.
>>>>> One usage of LTR encoding is to reduce error propagation for video
>>>>> transmission
>>>>> in packet lossy networks.  For example, encoder may want to specify
>>>>> some key frames as
>>>>> LTR pictures and use them as reference frames for encoding. With
>>>>> extra protection
>>>>> selectively on these LTR frames or synchronization with the 
>>>>> receiver
>>>>> of reception of
>>>>> the LTR frames during transmission, decoder can receive reference
>>>>> frames more reliably
>>>>> than other non-reference frames. As a result, transmission error 
>>>>> can
>>>>> be effectively
>>>>> restricted within certain frames rather than propagated to future
>>>>> frames.
>>>>> 
>>>>> We are introducing below V4l2 Controls for this feature
>>>>> 1. V4L2_CID_MPEG_VIDEO_LTRCOUNT
>>>>>     a. This is used to query or configure the number of LTR frames.
>>>>>        This is a static control and is controlled by the client.
>>>>>     b. The LTR index varies from 0 to the max LTR-1.
>>>>>     c. If LTR Count is more than max supported LTR count (max LTR) 
>>>>> by
>>>>> driver, it will be rejected.
>>>>>     d. Auto Marking : If LTR count is non zero,
>>>>>         1) first LTR count frames would be mark as LTR 
>>>>> automatically
>>>>> after
>>>>>       	   every IDR frame (inclusive).
>>>>>         2) For multilayer encoding: first LTR count base layer
>>>>> reference frames starting after
>>>>>            every IDR frame (inclusive) in encoding order would be
>>>>> marked as LTR frames by the encoder.
>>>>>         3) Auto marking of LTR due to IDR should consider following
>>>>> conditions:
>>>>>             1. The frame is not already set to be marked as LTR.
>>>>>             2. The frame is part of the base layer in the
>>>>> hierarchical layer case.
>>>>>             3. The number of frames currently marked as LTR is less
>>>>> than the maximum LTR frame index plus 1.
>>>>>     e. Encoder needs to handle explicit Mark/Use command when 
>>>>> encoder
>>>>> is still doing "auto" marking
>>> 
>>> I don't follow this, quite possibly due to lack of experience with
>>> encoders.
>>> 
>>> I kind of would expect to see two modes: either automatic where
>>> encoders can
>>> mark up to LTR_COUNT frames as long term reference, and userspace 
>>> just
>>> sets
>>> LTR_COUNT and doesn't have to do anything else.
>>> 
>>> Or it is manual mode where userspace explicitly marks long term
>>> reference
>>> frames.
>>> 
>>> From the proposal above it looks like you can mix auto and manual
>>> modes.
>>> 
>>> BTW, how do you 'unmark' long term reference frames?
>>> 
>>> This feature is for stateful encoders, right?
>>> 
>>>> 
>>>> Perhaps we are missing a LONG_TERM_REFERENCE_MODE ? I bet some 
>>>> encoder
>>>> can select by themself long term references and even some encoders 
>>>> may
>>>> not let the user decide.
>>>> 
>>>> (not huge han of LTR acronyme, but that could be fine too, assuming
>>>> you
>>>> add more _).
>>>> 
>> 
>> Userspace sets LTR count which signifies the number of LTR frames
>> encoder needs to generate or keep.
>> The encoder has to build-up its internal buffer reference list (aka 
>> DBP
>> list or recon buffer list).
>> In order to achieve that encoder will fill It's LTR (long term
>> references) list and STR (short term references) list
>> by auto marking n frames as LTR frames(n is equal to LTR count) based 
>> on
>> auto-marking dictated by the encoder spec.
>> The client then can replace those automatically marked frames with new
>> frames using V4L2_CID_MPEG_VIDEO_MARKLTRFRAME and can ask
>> encoder to refer the newly marked frame for encoding the next frame
>> using V4L2_CID_MPEG_VIDEO_USELTRFRAME.
>> 
>>>>> 
>>>>> 2. V4L2_CID_MPEG_VIDEO_MARKLTRFRAME :
>>>>>     a. This signals to mark the current frame as LTR frame. It is a
>>>>> dynamic control and also provide the LTR index to be used.
>>>>>     b. the LTR index provided by this control should never exceed 
>>>>> the
>>>>> max LTR-1. Else it will be rejected.
>>>> 
>>>> The "current" frame seems a bit loose. Perhaps you wanted to use
>>>> buffer
>>>> flags ? A bit like what we have to signal TOP/BOTTOM fields in
>>>> alternate interlacing.
>>> 
>>> I was thinking the same thing. Using a control for this doesn't seem
>>> right.
>>> 
>> 
>> the client sets this to replace automatically marked frames by the
>> encoder with a particular frame.
>> this provides an index that ranges from 0 to LTR count-1 and then the
>> particular frame will be marked with that index.
>> this can be achieved through request by associating this control with 
>> a
>> specific buffer to make it synchronized.
>> 
>>>> 
>>>>> 
>>>>> 3. V4L2_CID_MPEG_VIDEO_USELTRFRAME :
>>>>>     a. This specifies the LTR frame(s) to be used for encoding the
>>>>> current frame. This is a dynamic control.
>>>>>     b. LTR Use Bitmap : this consists of bits [0, 15]. A total of N
>>>>> LSB bits of this field are valid,
>>>>>        where N is the maximum number of LTRs supported. All the 
>>>>> other
>>>>> bits are invalid and should be rejected.
>>>>>        The LSB corresponds to the LTR index 0. Bit N-1 from the LSB
>>>>> corresponds to the LTR index max LTR-1.
>>> 
>>> How would userspace know this? Esp. with auto marking since userspace
>>> would have
>>> to predict how auto marking works (if I understand this correctly).
>>> 
>> 
>> Client sets LTR count which tells about the number of LTR frames
>> automatically marked by the encoder.
>> so client can use LTR index (0 to LTR count -1) to ask encoder to 
>> refer
>> any particular
>> frame (marked automatically by driver or marked by client with
>> V4L2_CID_MPEG_VIDEO_MARKLTRFRAME) as a reference to encode the next
>> frame.
>> 
>>> For which HW encoder is this meant?
>>> 
>> This is primarily meant for H264 and HEVC.
> 
> The venus encoder?
yes
> 
> Some more questions:
> 
> 1) How many LTR frames do h.264 and hevc allow?
According to spec, MAX LTR allowed by H.264 is 15 and HEVC allows max 32 
LTR frames.
> 2) Given N LTR frames, is there a ordering of those frames? E.g.
>    the LTR frame with index 0 is processed/used differently from
>    LTR frame with index 1? Or are they all equal in that it is just a 
> pool
>    of LTR frames that the encoder can use as it wishes?
they are different frames marked to be used as LTR and stored in 
available indices.
Userspace notifies encoder which LTR frame index to use via USE LTR 
control.

Thanks,
Dikshita
> 
> Regards,
> 
> 	Hans
> 
>> 
>> Thanks,
>> Dikshita
>> 
>>>> 
>>>> Note, I haven't captured very well the userspace control flow, 
>>>> perhaps
>>>> this could be enhanced through writing some documentation.
>>>> 
>>>> As per all other generic encoder controls, we need to make sure it
>>>> will
>>>> be usable and flexible enough for multiple HW blocks, as it can be
>>>> tedious to extend later otherwise. It is important that along with
>>>> this
>>>> RFC you provide some comparisons with with other HW / SW APIs in 
>>>> order
>>>> to help justify the design decisions. I also think there should be
>>>> link made V4L2_CID_MPEG_VIDEO_GOP_* , number of B-Frames etc.
>>> 
>>> I agree with Nicolas.
>>> 
>>> Regards,
>>> 
>>> 	Hans
>>> 
>>>> 
>>>> regards,
>>>> Nicolas
>>>> 
>>>>> 
>>>>> Dikshita Agarwal (1):
>>>>>   media: v4l2-ctrls:  add control for ltr
>>>>> 
>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c | 6 ++++++
>>>>>  include/uapi/linux/v4l2-controls.h   | 4 ++++
>>>>>  2 files changed, 10 insertions(+)
>>>>> 
>>>>