[FFmpeg-devel] [FFmpeg-cvslog] lavc/dxva2_h264: Fix incorrect assert statement.

[Matt Oliver]

On 16 March 2016 at 23:10, Hendrik Leppkes <h.leppkes at gmail.com> wrote:

> On Wed, Mar 16, 2016 at 1:04 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Wed, 16 Mar 2016 22:55:09 +1100
> > Matt Oliver <protogonoi at gmail.com> wrote:
> >
> >> On 16 March 2016 at 22:32, Clément Bœsch <u at pkh.me> wrote:
> >>
> >> > On Wed, Mar 16, 2016 at 12:31:35PM +0100, Matt Oliver wrote:
> >> > > ffmpeg | branch: master | Matt Oliver <protogonoi at gmail.com> | Wed
> Mar
> >> > 16 22:28:29 2016 +1100| [109dfed7fc265f3e071854d5e6de5dd7f82ff9fb] |
> >> > committer: Matt Oliver
> >> > >
> >> > > lavc/dxva2_h264: Fix incorrect assert statement.
> >> > >
> >> > > Signed-off-by: Matt Oliver <protogonoi at gmail.com>
> >> > >
> >> > > >
> >> >
> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=109dfed7fc265f3e071854d5e6de5dd7f82ff9fb
> >> > > ---
> >> > >
> >> > >  libavcodec/dxva2_h264.c |    7 ++++++-
> >> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/libavcodec/dxva2_h264.c b/libavcodec/dxva2_h264.c
> >> > > index 61cce3a..54f2b80 100644
> >> > > --- a/libavcodec/dxva2_h264.c
> >> > > +++ b/libavcodec/dxva2_h264.c
> >> > > @@ -426,7 +426,12 @@ static int
> >> > commit_bitstream_and_slice_buffer(AVCodecContext *avctx,
> >> > >          slice_data = ctx_pic->slice_long;
> >> > >          slice_size = ctx_pic->slice_count *
> >> > sizeof(*ctx_pic->slice_long);
> >> > >      }
> >> > > -    assert((bs->DataSize & 127) == 0);
> >> > > +#if CONFIG_D3D11VA
> >> > > +    assert((((D3D11_VIDEO_DECODER_BUFFER_DESC *)bs)->DataSize &
> 127) ==
> >> > 0);
> >> > > +#endif
> >> > > +#if CONFIG_DXVA2
> >> > > +    assert((((DXVA2_DecodeBufferDesc *)bs)->DataSize & 127) == 0);
> >> > > +#endif
> >> >
> >> > please use av_assert*
> >>
> >>
> >> My apologies, I just modified the existing assert usage so that it
> doesnt
> >> generate a compile error (which I thought was a rather trivial patch).
> >> There are other uses of assert instead of av_assert in the same file, do
> >> you want an additional patch that changes all uses (although im sure
> there
> >> are many other locations that use assert still)
> >
> > In addition to this, is this even correct? Both D3D11VA and DXVA2 can
> > be defined AFAIK.
>
> This patch is indeed not correct. Please submit patches for review on
> the ML in the future for components you do not maintain.


My bad. The previous code did not compile as type for bs was a typedef for
void* and so there is no DataSize element. This has been broken for a while
without anyone realising as noone tested a debug build.

I missed adding the pixel format conditionals as I made the assumption that
if these were not true then given that this function is only ever called
from ff_dxva2_common_end_frame and if neither of those conditionals are met
then both bs and sc would be passed as NULL. I made the assumption given
that the original assert in the original code didnt check for possibility
of being NULL that this was therefore originally considered to not be an
issue. Obviously I gave the original code a bit to much credit. So the
patch was more about fixing a compilation error in the code rather than
fixing that and an existing logic error that I didnt see. So your right I
missed the other half, mea culpa, thanks Hendrik for fixing the logic error
as well.