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

[Zhao, Yakui]

On Fri, 2014-05-30 at 03:30 -0600, Balachandran, Sreerenj wrote:
> 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 :)

OK. I will add more comment to avoid the misleading.

> 
> 
> 
> >
> >>
> >>>       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?
> 

Yes. I prefer the option 1.

> 
> 
> > 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] =
> >
>