[Libva] [RFC PATCH Libva-intel-driver 1/6] H264_Encoding: Parse the packed header data from user to fix the hacked code of HW skip bytes
Zhao, Yakui
yakui.zhao at intel.com
Wed May 21 00:27:20 PDT 2014
On Wed, 2014-05-21 at 00:57 -0600, Zhao, Yakui wrote:
> 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 <yakui.zhao at intel.com>:
> >
> > > 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 <yakui.zhao at intel.com>
> > > ---
> > > 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.
Sorry for the out-of-date spec in my hand. I will add it.
>
> >
> > 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
> > > Libva at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/libva
>
>
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libva
More information about the Libva
mailing list