[Mesa-dev] [PATCH] gallium: Add support for frame_cropping_flag of VAEncSequenceParameterBufferH264

[Liu, Leo]

On 4/9/19 12:23 AM, Sahu, Satyajit wrote:
> On 4/9/2019 12:34 AM, Liu, Leo wrote:
>> On 4/8/19 1:17 AM, Sahu, Satyajit wrote:
>>> From: suresh guttula <suresh.guttula at amd.com>
>>>
>>> This patch will add support for frame_cropping when the input size is not
>>> matched with aligned size. Currently vaapi driver ignores frame cropping
>>> values provided by client. This change will update SPS nalu with proper
>>> cropping values.
>>>
>>> Signed-off-by: suresh guttula <suresh.guttula at amd.com>
>>> Signed-off-by: Satyajit Sahu <satyajit.sahu at amd.com>
>>> ---
>>>     src/gallium/drivers/radeon/radeon_vce_52.c       | 8 ++++++--
>>>     src/gallium/include/pipe/p_video_state.h         | 5 +++++
>>>     src/gallium/state_trackers/va/picture_h264_enc.c | 8 ++++++++
>>>     3 files changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/radeon/radeon_vce_52.c b/src/gallium/drivers/radeon/radeon_vce_52.c
>>> index fc7ddc62a90..593730756d5 100644
>>> --- a/src/gallium/drivers/radeon/radeon_vce_52.c
>>> +++ b/src/gallium/drivers/radeon/radeon_vce_52.c
>>> @@ -81,8 +81,12 @@ static void get_pic_control_param(struct rvce_encoder *enc, struct pipe_h264_enc
>>>     	unsigned encNumMBsPerSlice;
>>>     	encNumMBsPerSlice = align(enc->base.width, 16) / 16;
>>>     	encNumMBsPerSlice *= align(enc->base.height, 16) / 16;
>>> -	enc->enc_pic.pc.enc_crop_right_offset = (align(enc->base.width, 16) - enc->base.width) >> 1;
>>> -	enc->enc_pic.pc.enc_crop_bottom_offset = (align(enc->base.height, 16) - enc->base.height) >> 1;
>> Shouldn't we need to keep this code path when the cropping info is not
>> available, since VA spec says it's optional.
>>
>> Regards,
>>
>> Leo
> As per H264 standard
>
> When frame_cropping_flag is equal to 0, the values of
> frame_crop_left_offset, frame_crop_right_offset, frame_crop_top_offset,
> and frame_crop_bottom_offset shall be inferred to be equal to 0.
> (https://www.itu.int/rec/T-REC-H.264)
>
> So I think it should be under frame_cropping_flag condition.

For OMX encode, gst-omx haven't passed any cropping information to 
driver, that's why the alignment was used as cropping for certain size 
video, we should keep the code path for OMX.

Also you should separate the patch to three, titled as for vl:, 
radeon/vce:, and st/va/enc:


Leo


>
> Regards,
>
> Satyajit
>
>>
>>
>>> +	if (pic->pic_ctrl.enc_frame_cropping_flag) {
>>> +		enc->enc_pic.pc.enc_crop_left_offset = pic->pic_ctrl.enc_frame_crop_left_offset;
>>> +		enc->enc_pic.pc.enc_crop_right_offset = pic->pic_ctrl.enc_frame_crop_right_offset;
>>> +		enc->enc_pic.pc.enc_crop_top_offset = pic->pic_ctrl.enc_frame_crop_top_offset;
>>> +		enc->enc_pic.pc.enc_crop_bottom_offset = pic->pic_ctrl.enc_frame_crop_bottom_offset;
>>> +	}
>>>     	enc->enc_pic.pc.enc_num_mbs_per_slice = encNumMBsPerSlice;
>>>     	enc->enc_pic.pc.enc_b_pic_pattern = MAX2(enc->base.max_references, 1) - 1;
>>>     	enc->enc_pic.pc.enc_number_of_reference_frames = MIN2(enc->base.max_references, 2);
>>> diff --git a/src/gallium/include/pipe/p_video_state.h b/src/gallium/include/pipe/p_video_state.h
>>> index 05855a36e23..1369f1a8ca6 100644
>>> --- a/src/gallium/include/pipe/p_video_state.h
>>> +++ b/src/gallium/include/pipe/p_video_state.h
>>> @@ -395,6 +395,11 @@ struct pipe_h264_enc_pic_control
>>>     {
>>>        unsigned enc_cabac_enable;
>>>        unsigned enc_constraint_set_flags;
>>> +   unsigned enc_frame_cropping_flag;
>>> +   unsigned enc_frame_crop_left_offset;
>>> +   unsigned enc_frame_crop_right_offset;
>>> +   unsigned enc_frame_crop_top_offset;
>>> +   unsigned enc_frame_crop_bottom_offset;
>>>     };
>>>     
>>>     struct pipe_h264_enc_picture_desc
>>> diff --git a/src/gallium/state_trackers/va/picture_h264_enc.c b/src/gallium/state_trackers/va/picture_h264_enc.c
>>> index abfd39633de..f46b3425566 100644
>>> --- a/src/gallium/state_trackers/va/picture_h264_enc.c
>>> +++ b/src/gallium/state_trackers/va/picture_h264_enc.c
>>> @@ -127,6 +127,14 @@ vlVaHandleVAEncSequenceParameterBufferTypeH264(vlVaDriver *drv, vlVaContext *con
>>>        context->desc.h264enc.rate_ctrl.frame_rate_num = h264->time_scale / 2;
>>>        context->desc.h264enc.rate_ctrl.frame_rate_den = h264->num_units_in_tick;
>>>        context->desc.h264enc.pic_order_cnt_type = h264->seq_fields.bits.pic_order_cnt_type;
>>> +
>>> +   if (h264->frame_cropping_flag) {
>>> +      context->desc.h264enc.pic_ctrl.enc_frame_cropping_flag = h264->frame_cropping_flag;
>>> +      context->desc.h264enc.pic_ctrl.enc_frame_crop_left_offset = h264->frame_crop_left_offset;
>>> +      context->desc.h264enc.pic_ctrl.enc_frame_crop_right_offset = h264->frame_crop_right_offset;
>>> +      context->desc.h264enc.pic_ctrl.enc_frame_crop_top_offset = h264->frame_crop_top_offset;
>>> +      context->desc.h264enc.pic_ctrl.enc_frame_crop_bottom_offset = h264->frame_crop_bottom_offset;
>>> +   }
>>>        return VA_STATUS_SUCCESS;
>>>     }
>>>