On Wed, 2014-05-21 at 00:26 -0600, Gwenole Beauchesne wrote: > Hi Yakui, > > Thanks for working on this. Additional comments.
Thanks for your comment. > > 2014-05-21 7:51 GMT+02:00 Zhao, Yakui <[email protected]>: > > > When the packed header data from user is inserted into the coded clip, it > > uses > > the hacked code to check the number of HW skip emulation bytes. This is > > wrong. > > So fix it. > > Of course if the packed header data is generated by the driver, it is > > unnecessary to check it and it can still use the pre-defined number of HW > > skip bytes. > > > > Signed-off-by: Zhao Yakui <[email protected]> > > --- > > src/gen6_mfc_common.c | 58 > > ++++++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 55 insertions(+), 3 deletions(-) > > > > diff --git a/src/gen6_mfc_common.c b/src/gen6_mfc_common.c > > index 33b9d55..f497e75 100644 > > --- a/src/gen6_mfc_common.c > > +++ b/src/gen6_mfc_common.c > > @@ -405,6 +405,53 @@ void intel_mfc_brc_prepare(struct encode_state > > *encode_state, > > } > > } > > > > +static int intel_avc_find_skipemulcnt(unsigned char *buf, int bits_length) > > +{ > > + int i, found; > > + int leading_zero_cnt, byte_length, zero_byte; > > + int nal_unit_type; > > + int skip_cnt = 0; > > + > > +#define NAL_UNIT_TYPE_MASK 0x1f > > + > > + byte_length = ALIGN(bits_length, 32) >> 3; > > + > > + > > + leading_zero_cnt = 0; > > + found = 0; > > + for(i = 0; i < byte_length - 4; i++) { > > + if (((buf[i] == 0) && (buf[i + 1] == 0) && (buf[i + 2] == 1)) || > > + ((buf[i] == 0) && (buf[i + 1] == 0) && (buf[i + 2] == 0) && > > (buf[i + 3] == 1))) { > > + found = 1; > > + break; > > + } > > + leading_zero_cnt++; > > + } > > + if (!found) { > > + /* warning message is complained. But anyway it will be inserted. > > */ > > + WARN_ONCE("Invalid packed header data. " > > + "Can't find the 000001 start_prefix code\n"); > > + return 0; > > + } > > + i = leading_zero_cnt; > > + > > + zero_byte = 0; > > + if (!((buf[i] == 0) && (buf[i + 1] == 0) && (buf[i + 2] == 1))) > > + zero_byte = 1; > > + > > + skip_cnt = leading_zero_cnt + zero_byte + 3; > > + > > + /* the unit header byte is accounted */ > > + nal_unit_type = (buf[skip_cnt]) & NAL_UNIT_TYPE_MASK; > > + skip_cnt += 1; > > + > > + if (nal_unit_type == 14 || nal_unit_type == 20) { > > + /* more unit header bytes are accounted for MVC/SVC */ > > + skip_cnt += 3; > > + } > > + return skip_cnt; > > +} > > + > > void intel_mfc_avc_pipeline_header_programing(VADriverContextP ctx, > > struct encode_state > > *encode_state, > > struct intel_encoder_context > > *encoder_context, > > Well, actually, what I meant was: > 1. Skip the leading zeros. Really, i.e. even for the part submitted to the HW. > 2. Start counting bytes to skip from this new offset. > > Reason: the HW supports a limited range of Skip emul bytes. So, if you > skip the leading zeros, you can always fulfill this condition. > Otherwise, you risk an overflow. [Yakui]: What you mentioned is an issue if the user add a lot of leading_zero_8bits which is beyond the HW range(I also consider this scenario). But at most cases it is a corner case. So I don't add a lot of check/adjustment for this corner case. Of course I can add some warning info so that the user-space app can get it. What do you think? > > Since it is a VA-API requirement for packed headers to contain the > start code prefix, you could split that up into: > 1. Search for the start code prefix (sc_offset) ; > 2. Determine the nalUnitHeaderBytes count. > > Based on (1), you can safely make the sc_offset the new origin of the > buffer to submit + reduce the programmed size accordingly. > And (1) could also be shared with other codecs. [Yakui]: After this is added, it will have no issue that the returned value is beyond the limited range of skip emul bytes. But after adjusting the offset/filled size, this will be different with the content passed from the user(for example: some specific symbols). Maybe this is not what they expected. So I use the solution that simply inserts the packed rawdata and don't skip the corresponding leading_zero_bits. > > Another thing, also include MVCD. i.e. nal_unit_type == 21, IIRC. [Yakui]: Really? It seems that this is one reserved type from the H264 spec. > > Thanks, > Gwenole. > > > @@ -413,6 +460,7 @@ void > > intel_mfc_avc_pipeline_header_programing(VADriverContextP ctx, > > struct gen6_mfc_context *mfc_context = encoder_context->mfc_context; > > int idx = va_enc_packed_type_to_idx(VAEncPackedHeaderH264_SPS); > > unsigned int rate_control_mode = encoder_context->rate_control_mode; > > + unsigned int skip_emul_byte_cnt; > > > > if (encode_state->packed_header_data[idx]) { > > VAEncPackedHeaderParameterBuffer *param = NULL; > > @@ -423,12 +471,13 @@ void > > intel_mfc_avc_pipeline_header_programing(VADriverContextP ctx, > > param = (VAEncPackedHeaderParameterBuffer > > *)encode_state->packed_header_param[idx]->buffer; > > length_in_bits = param->bit_length; > > > > + skip_emul_byte_cnt = intel_avc_find_skipemulcnt((unsigned char > > *)header_data, length_in_bits); > > mfc_context->insert_object(ctx, > > encoder_context, > > header_data, > > ALIGN(length_in_bits, 32) >> 5, > > length_in_bits & 0x1f, > > - 5, /* FIXME: check it */ > > + skip_emul_byte_cnt, > > 0, > > 0, > > !param->has_emulation_bytes, > > @@ -446,12 +495,14 @@ void > > intel_mfc_avc_pipeline_header_programing(VADriverContextP ctx, > > param = (VAEncPackedHeaderParameterBuffer > > *)encode_state->packed_header_param[idx]->buffer; > > length_in_bits = param->bit_length; > > > > + skip_emul_byte_cnt = intel_avc_find_skipemulcnt((unsigned char > > *)header_data, length_in_bits); > > + > > mfc_context->insert_object(ctx, > > encoder_context, > > header_data, > > ALIGN(length_in_bits, 32) >> 5, > > length_in_bits & 0x1f, > > - 5, /* FIXME: check it */ > > + skip_emul_byte_cnt, > > 0, > > 0, > > !param->has_emulation_bytes, > > @@ -469,12 +520,13 @@ void > > intel_mfc_avc_pipeline_header_programing(VADriverContextP ctx, > > param = (VAEncPackedHeaderParameterBuffer > > *)encode_state->packed_header_param[idx]->buffer; > > length_in_bits = param->bit_length; > > > > + skip_emul_byte_cnt = intel_avc_find_skipemulcnt((unsigned char > > *)header_data, length_in_bits); > > mfc_context->insert_object(ctx, > > encoder_context, > > header_data, > > ALIGN(length_in_bits, 32) >> 5, > > length_in_bits & 0x1f, > > - 5, /* FIXME: check it */ > > + skip_emul_byte_cnt, > > 0, > > 0, > > !param->has_emulation_bytes, > > -- > > 1.7.12-rc1 > > > > _______________________________________________ > > Libva mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/libva _______________________________________________ Libva mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libva
