[Libva] [PATCH 2/3] Encode: Don't mess up the index handling of packed raw data with packed slice header

[Sreerenj]

On 30.05.2014 11:48, Zhao, Yakui wrote:
> On Fri, 2014-05-30 at 11:06 +0300, Sreerenj wrote:
>> On 29.05.2014 03:42, Zhao, Yakui wrote:
>>> On Wed, 2014-05-28 at 15:02 -0600, sreerenj.balachandran at intel.com
>>> wrote:
>>>> From: Sreerenj Balachandran <sreerenj.balachandran at intel.com>
>>>>
>>>> Avoid storing packed slice header index as packed raw data index.
>>>> This patch is a preparation for submitting all the packed slice
>>>> headers as a group , instead of pairing with VAEncSliceParameterBuffer in
>>>> multi slice per frame encoding. If the user only passes the packed
>>>> slice_header data for one slice before VAEncSliceParameterbuffer
>>>> one by one, it still can work without this patch.
>>> Hi, Sreerenj
>>>
>>>       Thanks for your patch.
>>>       But I have two concerns about this patch and following patch(grouped
>>> submission of packed_slice header)
>>>       a. After this patch, it won't work when passing the packed
>>> rawdata/slice header like the below order for one slice(Of course they
>>> are passed before VAEncSliceParameterBuffer).
>>>          >packed rawdata 0, slice_header , packed rawdata 1;
>>>          
>>>          In such case the packed rawdata 1 won't be inserted as expected.
>>>
>>>          Without this patch, it will firstly insert packed rawdata 0/1 and
>>> then insert the slice_header.
>>>
>>>          If so, it will be better that a new structure is added to store
>>> the packed slice_header data.
>>
>> I didn't understand the issue here. With this patch i have not changed
>> any logic i guess. isn't it? The index value num_slice_params_ext will
>> get incremented only when the driver receives
>> the VAEncSliceParameterBuffer. If I understood correctly you were
>> talking about a case having two packed_raw_data and one slice_header
>> before the submission of VAEncSliceParameterBuffer.
>> I haven't seen any issue. The packed_raw_data_0 will inserted to
>> slice_rawdata_index[0] then the packed_slice_header will be inserted to
>> slice_header_index[0] , finally the slice_raw_data_1 will
>> be inserted to slice_rawdata_index[1].  isn't it ?
> But the current logic is related with the function of
> intel_avc_slice_insert_packed_data.  Now the packed rawdata &
> slice_header data are tracked by using the
> packed_header_params_ext/packed_header_data_ext.(The packed slice_header
> data is one specific type of packed rawdata. But it still increases the
> slice_rawdata_count for the given slice).
>
> The below is the rough flowchart about how to insert the packed data.
>     1. use the slice_rawdata_index/count to enumerate and insert the
> corresponding packed_data. But the slice_header is skipped in this step,
> which is to assure that slice_header is lastly inserted for one slice.
>     2. use the slice_header_index to insert the packed slice_header data.
>
> After this patch, the slice_rawdata_count is different for the below
> sequence of "packed rawdata 0, packed slice_header 0, packed rawdata 1".


Okay I understood the issue. Thanks :) But this is really misleading . 
It would be good if you can write some comments at least.
Because the name raw_data_count implies the number of raw_datas and 
usually some one who read the code won't think it includes the count of 
some thing else too :)



>
>>
>>>       b. The next patch will try to group the submission of packed
>>> slice_header data for multi-slices. If the frame is divided into four
>>> slices and three slice header data are passed, how will it be handled?
>>> And how to decide which slice header data is inserted for one slice? As
>>> you know, the current mechanism is that the slice_header data is
>>> optional.(That is to say: For one slice the driver will firstly try to
>>> insert the slice_header data passed from user. If it doesn't exist, it
>>> will generate the data by itself.) So the submission of slice_header
>>> data is also based on the VAEncSliceParameterBuffer delimeter.
>>
>>
>> I too thought about this case before the implementation.AFAIK we are not
>> supposed to impose any ordering rule for packed_slice_headers, since as
>> per the va specification the only packed_header having order restriction
>> is  Packed Raw Data. So the scenario should work like this:  If the user
>> configured the driver to work with packed_slice_header then it is the
>> responsibility of user to submit all slice_headers. On the other
>> hand if the user have created the VAConfig with out PACKED_SLICE_HEADER
>> support then he is not supposed to provide any packed_slice_header.
>> Which means the user is not allowed
>> to use a mixed mode like " four slices and three packed slice headers".
>> What do you think?
> This proposal of the PACKED_SLICE_HEADER restriction is OK to me.
>
> But not sure whether the current implementation is still acceptable to
> you and meet with your requirement. (The packed slice_header for every
> slice is still based on VAEncSliceParameterBuffer delimeter. Not use the
> grouped submission of all slice_headers before
> VAEncSliceParameterBuffer).


So options here:
option1:  restrict the submission of packed_slice_headers such that it 
should be aligned with VAEncSliceParamerterBuffer and add it to the va 
specification. Which mean no support for grouped submission of 
packed_slice_headers for each frame.
option2: add support for grouped slice headers . No addition to the va spec.

I guess you are preferring to follow option1.  right?



> Thanks.
>      Yakui
>
>>
>>
>>> Thanks.
>>>       Yakui
>>>
>>>> ---
>>>>    src/i965_drv_video.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c
>>>> index 44da864..4854a62 100755
>>>> --- a/src/i965_drv_video.c
>>>> +++ b/src/i965_drv_video.c
>>>> @@ -2341,8 +2341,7 @@ i965_encoder_render_picture(VADriverContextP ctx,
>>>>                    vaStatus = VA_STATUS_ERROR_INVALID_PARAMETER;
>>>>                    return vaStatus;
>>>>                }
>>>> -            if (encode->last_packed_header_type == VAEncPackedHeaderRawData ||
>>>> -                encode->last_packed_header_type == VAEncPackedHeaderSlice) {
>>>> +            if (encode->last_packed_header_type == VAEncPackedHeaderRawData) {
>>>>                    vaStatus = I965_RENDER_ENCODE_BUFFER(packed_header_data_ext);
>>>>                    if (vaStatus == VA_STATUS_SUCCESS) {
>>>>                        /* store the first index of the packed header data for current slice */
>>>> @@ -2351,6 +2350,10 @@ i965_encoder_render_picture(VADriverContextP ctx,
>>>>                                 SLICE_PACKED_DATA_INDEX_TYPE | (encode->num_packed_header_data_ext - 1);
>>>>                        }
>>>>                        encode->slice_rawdata_count[encode->num_slice_params_ext]++;
>>>> +                }
>>>> +	    } else if (encode->last_packed_header_type == VAEncPackedHeaderSlice) {
>>>> +                vaStatus = I965_RENDER_ENCODE_BUFFER(packed_header_data_ext);
>>>> +                if (vaStatus == VA_STATUS_SUCCESS) {
>>>>                        if (encode->last_packed_header_type == VAEncPackedHeaderSlice) {
>>>>                            if (encode->slice_header_index[encode->num_slice_params_ext] == 0) {
>>>>                                encode->slice_header_index[encode->num_slice_params_ext] =
>

-- 
Thanks
Sree

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.