[FFmpeg-devel] [PATCH] lavc/vaapi_encode: Don't pass VAConfigAttribEncPackedHeaders with value set to 0

[Xiang, Haihao]

On Tue, 2018-03-06 at 14:42 +0000, Mark Thompson wrote:
> On 06/03/18 06:04, Eoff, Ullysses A wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> > > Mark Thompson
> > > Sent: Tuesday, February 13, 2018 11:54 AM
> > > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: Don't pass
> > > VAConfigAttribEncPackedHeaders with value set to 0
> > > 
> > > On 13/02/18 18:52, Mark Thompson wrote:
> > > > On 13/02/18 08:24, Haihao Xiang wrote:
> > > > > Recent Intel i965 driver commit strictly disallows application to set
> > > > > unsupported attribute values, VA_ENC_PACKED_HEADER_NONE (0) is not
> > > > > used
> > > > > in Intel i965 driver, so application shouldn't pass this value to the
> > > > > driver. On the other hand, VA_ENC_PACKED_HEADER_NONE (0) means the
> > > > > driver doesn't support any packed header mode, so application also
> > > > > shouldn't pass packed header to driver if a driver returns
> > > > > VA_ENC_PACKED_HEADER_NONE (0), the driver should work without
> > > > > VAConfigAttribEncPackedHeaders set for this case.
> > > > > 
> > > > > In addition, VA_ATTRIB_NOT_SUPPORTED and VA_ENC_PACKED_HEADER_NONE
> > > > > make
> > > > > thing messy, we will deprecate VA_ENC_PACKED_HEADER_NONE in the
> > > > > future. See https://github.com/intel/libva/issues/178 for more
> > > > > information.
> > > > > 
> > > > > This fixes broken vp9 encoder on Kably Lake with Intel I965 driver.
> > > > > 
> > > > > Signed-off-by: Haihao Xiang <haihao.xiang at intel.com>
> > > > > ---
> > > > >  libavcodec/vaapi_encode.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> > > > > index e371f5761ee..1d30aabed40 100644
> > > > > --- a/libavcodec/vaapi_encode.c
> > > > > +++ b/libavcodec/vaapi_encode.c
> > > > > @@ -1111,6 +1111,10 @@ static av_cold int
> > > > > vaapi_encode_config_attributes(AVCodecContext *avctx)
> > > > >                         ctx->va_packed_headers, attr[i].value);
> > > > >                  ctx->va_packed_headers &= attr[i].value;
> > > > >              }
> > > > > +
> > > > > +            if (!ctx->va_packed_headers)
> > > > > +                continue;
> > > > > +
> > > > >              ctx->config_attributes[ctx->nb_config_attributes++] =
> > > > >                  (VAConfigAttrib) {
> > > > >                  .type  = VAConfigAttribEncPackedHeaders,
> > > > > 
> > > > 
> > > > This seems wrong to me: the driver has indicated that packed headers are
> > > > supported, so the user is providing this attribute to
> > > 
> > > indicate to the driver that they will not use any of them.  Compare the
> > > VP8 case, where the driver does not support them and says so -
> > > there we correctly don't provide the attribute (though maybe the
> > > commentary could be clearer on that).
> > > 
> > > Right, I hadn't realised you had already made a change so that encoding is
> > > currently broken.  I've made
> > > <https://github.com/intel/intel-vaapi-driver/pull/358> to revert the
> > > API/ABI-breaking part of the change.
> > > 
> > > Thanks,
> > > 
> > > - Mark
> > 
> > I prefer this patch over the one for intel-vaapi-driver.
> 
> Well, the driver patch should be applied anyway to fix the API/ABI break
> (existing libavcodec builds should continue to work on the new
> library/driver), but it can be reverted on the next major version bump.  Maybe
> a warning (i965_log_info()) could be added to the patch to indicate to the
> client that the usage is deprecated in libva 2 and will be removed in libva 3?
> 

Ok, I will merge your driver patch for this special case (allow 0 for
VAConfigAttribEncPackedHeaders when calling vaCreateConfig()) to resolve this
issue. Could you update your patch to add some warning message?

> > The va.h documentation for VA_ENC_PACKED_HEADER_NONE
> > says "Driver does not support any packed headers mode".
> > Hence, it's only valid for reporting to client that packed headers
> > are "unsupported".  Unfortunately, VA_ENC_PACKED_HEADER_NONE 
> > is redundant/ambiguous since there is also
> > VA_ATTRIB_NOT_SUPPORTED to relay the same information.
> > This is why we want to deprecate VA_ENC_PACKED_HEADER_NONE
> > in VAAPI.
> > 
> > I don't think it's correct for clients to send
> > VA_ENC_PACKED_HEADER_NONE attribute value to the driver
> > when the driver reports packed headers are "supported".  It
> > goes against VA_ENC_PACKED_HEADER_NONE's documented
> > purpose.  AFAIK, libavcodec is the only library that does this.  It
> > is better to just omit the attribute altogether if client does not
> > want to use any of the "supported" packed headers.  And this
> > patch solves that.
> 
> I still disagree with this logic.  The driver supplied a bitmask of allowed
> packed headers, and the client picks which of those it will send and supplies
> that back to the driver with vaCreateConfig().  If the driver believes that
> set is not sufficient then it can reject that choice, but if it is sufficient
> then the empty set should be just as much a valid setting as any other usable
> subset of the given headers.
> 
> Any talk of VA_ENC_PACKED_HEADER_NONE is orthogonal - that symbol isn't used
> in libavcodec at all, and the fact that it happens to have the same value
> (zero) as an empty bitmask is unfortunate but not relevant because one is only
> used in the driver -> client case (vaGetConfigAttributes()) while the other is
> only used in the client -> driver case (vaCreateConfig()).
> 

It will be better to update the va.h documentation for the value for the
VAConfigAttribEncPackedHeaders attribute when calling vaCreateConfig(). I prefer
 not to set VAConfigAttribEncPackedHeaders if VAConfigAttribEncPackedHeaders is
not supported by the driver or user application doesn't provide any valid packed
headers, and it should work with the old / new drivers.

> > In the future, it's probably worth amending VAAPI to allow for
> > drivers to relay when packed headers are required vs. optional,
> > too.
> 
> That sounds like a good idea, but the existing API does need to continue to
> work at least until a new major version is made.
> 

How about to use bit16-bit30 of the value to indicate a header is optional or
not? bit31 is defined as VA_ATTRIB_NOT_SUPPORTED. 

> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel