[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 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".

> 
> 
> >
> >      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). 

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