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.balachand...@intel.com > >>> wrote: > >>>> From: Sreerenj Balachandran <sreerenj.balachand...@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] = > > > _______________________________________________ Libva mailing list Libva@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libva