[FFmpeg-devel] avcodec/vaapi_h264: skip decode if pic has no slices

Submitted by Peter F on March 9, 2019, 8:30 p.m.

Details

Message ID CAMQ1+vzBwM6k8FZ67rNVzXJ5Wz=y72-WFqRjgqfe2mW=8_B2iA@mail.gmail.com
State New
Headers show

Commit Message

Peter F March 9, 2019, 8:30 p.m.
Thank you very much for your reply.
Am Sa., 9. März 2019 um 21:09 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:

> > From 386c94489a86bb747b6531f727843cf259a24f5d Mon Sep 17 00:00:00 2001
> > From: xbmc <fernetmenta@online.de>
> Is this author field meant to not have an actual name in it? Just verifying.

It can stay as is. The original author sometimes uses fernetmenta /
xbmc depending on his
local git configuration. The email is unique though. I just
transported it upstream and fixed
the minors.

>
> > Date: Sat, 26 Jan 2019 19:48:35 +0100
> > Subject: [PATCH] avcodec/vaapi_h264: skip decode if pic has no slices
>
> Something along the lines of "avcodec/vaapi_h264: skip decoding if no
> slices were provided"?
>
> Also I would prefer if the reasoning for skipping decode on our layer
> would be explained in further lines of the commit message, like you
> have nicely explained it in the initial e-mail (to work-around a mesa
> vaapi driver bug).
> I don't remember the specifics of AVC, but are there valid VCL NAL
> units without slices (and would such end up in this code path to begin
> with)? I would guess that there would be no such valid VCL NAL units
> (or if there were, they wouldn't get to this point in the decode
> chain) - mostly just noting that this might be something we would like
> to check to verify if this should be an error or a "normal" state.
>
> The general idea I'm OK with since if we already know that there's no
> slices to decode, we might as well skip decoding as long as that
> doesn't cause issues with the state of the underlying
> libraries/drivers. Thus, I would mostly just wait for a reply from one
> of the VAAPI wrapper maintainers regarding if this skip should happen
> here or earlier in the decode process (where the buffers are being
> allocated).
>

Yes I would also like to hear a statement from these guys. Especially as
we (and most likely everyone else) just uses the ffmpeg send / receive API
without feeling the need to introduce VAAPI workarounds in this generic
application code

Thanks again, please find attached the updated version

Peter

Comments

Mark Thompson March 13, 2019, 1 a.m.
On 09/03/2019 20:30, Peter F wrote:
> Thank you very much for your reply.
> Am Sa., 9. März 2019 um 21:09 Uhr schrieb Jan Ekström <jeebjp@gmail.com>:
> 
>>> From 386c94489a86bb747b6531f727843cf259a24f5d Mon Sep 17 00:00:00 2001
>>> From: xbmc <fernetmenta@online.de>
>> Is this author field meant to not have an actual name in it? Just verifying.
> 
> It can stay as is. The original author sometimes uses fernetmenta /
> xbmc depending on his
> local git configuration. The email is unique though. I just
> transported it upstream and fixed
> the minors.
> 
>>
>>> Date: Sat, 26 Jan 2019 19:48:35 +0100
>>> Subject: [PATCH] avcodec/vaapi_h264: skip decode if pic has no slices
>>
>> Something along the lines of "avcodec/vaapi_h264: skip decoding if no
>> slices were provided"?
>>
>> Also I would prefer if the reasoning for skipping decode on our layer
>> would be explained in further lines of the commit message, like you
>> have nicely explained it in the initial e-mail (to work-around a mesa
>> vaapi driver bug).
>> I don't remember the specifics of AVC, but are there valid VCL NAL
>> units without slices (and would such end up in this code path to begin
>> with)? I would guess that there would be no such valid VCL NAL units
>> (or if there were, they wouldn't get to this point in the decode
>> chain) - mostly just noting that this might be something we would like
>> to check to verify if this should be an error or a "normal" state.
>>
>> The general idea I'm OK with since if we already know that there's no
>> slices to decode, we might as well skip decoding as long as that
>> doesn't cause issues with the state of the underlying
>> libraries/drivers. Thus, I would mostly just wait for a reply from one
>> of the VAAPI wrapper maintainers regarding if this skip should happen
>> here or earlier in the decode process (where the buffers are being
>> allocated).

I suspect it would make sense for this to happen earlier in the decoder if it knows it doesn't have any slices (therefore not calling any hwaccel code at all), but I'm not really sure - maybe in this case it doesn't know early enough so you always have the start_frame call.  The current setup is going to allocate a reference frame but then with this change never do anything with it, which seems bad from an uninitialised-data perspective.  On the other hand, I agree that calling into the hardware decoder with no slices seems rather unhelpful, and it probably doesn't write anything useful to the frame either.

Still, all of the other hwaccel APIs (VDPAU, DXVA2, NVDEC) do currently do the same thing too, though, so it would seem nicer if this could be kept consistent across all of them.  If Mesa with VAAPI is the only case where this fails (checking Mesa with VDPAU on the same hardware might be interesting?) then fixing it in the driver would seem the best answer.  I might have a look at that later.

> From 3c4885579e86f6c002e614c4082a3bdb02d8426e Mon Sep 17 00:00:00 2001
> From: xbmc <fernetmenta@online.de>
> Date: Sat, 26 Jan 2019 19:48:35 +0100
> Subject: [PATCH] avcodec/vaapi_h264: skip decode if pic has no slices
> 
> This fixes / workarounds https://bugs.freedesktop.org/show_bug.cgi?id=105368.
> It was hit frequently when watching h264 channels received via DVB-X.
> Corresponding kodi bug: https://github.com/xbmc/xbmc/issues/15704
> ---
>  libavcodec/vaapi_h264.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c
> index 5854587a25..f12fdc457a 100644
> --- a/libavcodec/vaapi_h264.c
> +++ b/libavcodec/vaapi_h264.c
> @@ -317,6 +317,11 @@ static int vaapi_h264_end_frame(AVCodecContext *avctx)
>      H264SliceContext *sl = &h->slice_ctx[0];
>      int ret;
>  
> +    if (pic->nb_slices == 0) {
> +        ret = AVERROR_INVALIDDATA;
> +        goto finish;
> +    }
> +
>      ret = ff_vaapi_decode_issue(avctx, pic);
>      if (ret < 0)
>          goto finish;
> -- 
> 2.20.1

This pastes the parameter buffers you have already sent on to the front of the next frame, which is going to do something pretty weird - I think you want to call ff_vaapi_decode_cancel() before returning.

Have you tried this on the i965 and iHD drivers and made sure it doesn't do anything nasty in the same case there?  (Or if you can provide a sample file I'll have a go myself.)

Thanks,

- Mark
Peter F March 14, 2019, 10:05 a.m.
Hi Mark,
Am Mi., 13. März 2019 um 02:00 Uhr schrieb Mark Thompson <sw@jkqxz.net>:
[..]
> >>> Date: Sat, 26 Jan 2019 19:48:35 +0100
> >>> Subject: [PATCH] avcodec/vaapi_h264: skip decode if pic has no slices
> >>
> >> Something along the lines of "avcodec/vaapi_h264: skip decoding if no
> >> slices were provided"?
> >>
> >> Also I would prefer if the reasoning for skipping decode on our layer
> >> would be explained in further lines of the commit message, like you
> >> have nicely explained it in the initial e-mail (to work-around a mesa
> >> vaapi driver bug).
> >> I don't remember the specifics of AVC, but are there valid VCL NAL
> >> units without slices (and would such end up in this code path to begin
> >> with)? I would guess that there would be no such valid VCL NAL units
> >> (or if there were, they wouldn't get to this point in the decode
> >> chain) - mostly just noting that this might be something we would like
> >> to check to verify if this should be an error or a "normal" state.
> >>
> >> The general idea I'm OK with since if we already know that there's no
> >> slices to decode, we might as well skip decoding as long as that
> >> doesn't cause issues with the state of the underlying
> >> libraries/drivers. Thus, I would mostly just wait for a reply from one
> >> of the VAAPI wrapper maintainers regarding if this skip should happen
> >> here or earlier in the decode process (where the buffers are being
> >> allocated).
>
> I suspect it would make sense for this to happen earlier in the decoder if it knows it doesn't have any slices (therefore not calling any hwaccel code at all), but I'm not really sure - maybe in this case it doesn't know early enough so you always have the start_frame call.  The current setup is going to allocate a reference frame but then with this change never do anything with it, which seems bad from an uninitialised-data perspective.  On the other hand, I agree that calling into the hardware decoder with no slices seems rather unhelpful, and it probably doesn't write anything useful to the frame either.
>
> Still, all of the other hwaccel APIs (VDPAU, DXVA2, NVDEC) do currently do the same thing too, though, so it would seem nicer if this could be kept consistent across all of them.  If Mesa with VAAPI is the only case where this fails (checking Mesa with VDPAU on the same hardware might be interesting?) then fixing it in the driver would seem the best answer.  I might have a look at that later.
>
> > From 3c4885579e86f6c002e614c4082a3bdb02d8426e Mon Sep 17 00:00:00 2001
> > From: xbmc <fernetmenta@online.de>
> > Date: Sat, 26 Jan 2019 19:48:35 +0100
> > Subject: [PATCH] avcodec/vaapi_h264: skip decode if pic has no slices
> >
> > This fixes / workarounds https://bugs.freedesktop.org/show_bug.cgi?id=105368.
> > It was hit frequently when watching h264 channels received via DVB-X.
> > Corresponding kodi bug: https://github.com/xbmc/xbmc/issues/15704
> > ---
> >  libavcodec/vaapi_h264.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c
> > index 5854587a25..f12fdc457a 100644
> > --- a/libavcodec/vaapi_h264.c
> > +++ b/libavcodec/vaapi_h264.c
> > @@ -317,6 +317,11 @@ static int vaapi_h264_end_frame(AVCodecContext *avctx)
> >      H264SliceContext *sl = &h->slice_ctx[0];
> >      int ret;
> >
> > +    if (pic->nb_slices == 0) {
> > +        ret = AVERROR_INVALIDDATA;
> > +        goto finish;
> > +    }
> > +
> >      ret = ff_vaapi_decode_issue(avctx, pic);
> >      if (ret < 0)
> >          goto finish;
> > --
> > 2.20.1
>
> This pastes the parameter buffers you have already sent on to the front of the next frame, which is going to do something pretty weird - I think you want to call ff_vaapi_decode_cancel() before returning.
>
> Have you tried this on the i965 and iHD drivers and made sure it doesn't do anything nasty in the same case there?  (Or if you can provide a sample file I'll have a go myself.)

I tested a provided sample on vaapi (Intel Apollo Lake) with kodi and
the provided ffmpeg patch and it is still working correctly. Sadly it
seems upload.ffmpeg.org does not respond for me correctly (times out
on ipv6 and ipv4) - therefore I cannot upload the sample there. Do you
have another idea where to post the sample?

Best regards
Peter
>
> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

From 3c4885579e86f6c002e614c4082a3bdb02d8426e Mon Sep 17 00:00:00 2001
From: xbmc <fernetmenta@online.de>
Date: Sat, 26 Jan 2019 19:48:35 +0100
Subject: [PATCH] avcodec/vaapi_h264: skip decode if pic has no slices

This fixes / workarounds https://bugs.freedesktop.org/show_bug.cgi?id=105368.
It was hit frequently when watching h264 channels received via DVB-X.
Corresponding kodi bug: https://github.com/xbmc/xbmc/issues/15704
---
 libavcodec/vaapi_h264.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c
index 5854587a25..f12fdc457a 100644
--- a/libavcodec/vaapi_h264.c
+++ b/libavcodec/vaapi_h264.c
@@ -317,6 +317,11 @@  static int vaapi_h264_end_frame(AVCodecContext *avctx)
     H264SliceContext *sl = &h->slice_ctx[0];
     int ret;
 
+    if (pic->nb_slices == 0) {
+        ret = AVERROR_INVALIDDATA;
+        goto finish;
+    }
+
     ret = ff_vaapi_decode_issue(avctx, pic);
     if (ret < 0)
         goto finish;
-- 
2.20.1