[FFmpeg-devel] [PATCH] avcodec: disallow hwaccel with frame threads

[Hendrik Leppkes]

On Mon, Oct 26, 2015 at 1:49 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Sun, Oct 25, 2015 at 5:55 PM, Hendrik Leppkes <h.leppkes at gmail.com>
> wrote:
>
>> On Fri, Oct 23, 2015 at 1:54 PM, Hendrik Leppkes <h.leppkes at gmail.com>
>> wrote:
>> > HWAccels with frame threads are fundamentally flawed in avcodecs current
>> > design, and there are several known problems ranging from image
>> corruption
>> > to driver crashes.
>> >
>> > These problems come down to two design problems in the interaction of
>> > threads and HWAccel decoding:
>> >
>> > (1)
>> > While avcodec prevents parallel decoding and as such simultaneous access
>> > to the hardware accelerator from the decoding threads, it cannot account
>> > for the user code and its access to the hardware surfaces and the
>> hardware
>> > itself.
>> >
>> > This can result in image corruption or even driver crashes if the
>> > user code locks image surfaces while they are being used by the decoder
>> > threads as reference frames.
>> >
>> > The current HWAccel API does not offer any way to ensure exclusive access
>> > to the hardware or the surfaces if frame threading is used.
>> >
>> > (2)
>> > Initialization of the HWAccel with frame threads is non-trivial, and many
>> > decoders had and still have issues that cause excess calls to the
>> > get_format callback.
>> >
>> > This will potentially cause duplicate HWAccel initialization, which in
>> > extreme cases can even lead to driver crashes if the HWAccel is
>> > re-initialized while the user code is actively accessing the hardware
>> > surfaces associated with it, or lead to image corruption due to lost
>> > reference frames.
>> >
>> > While both of these issues are solvable, fixing (1) would at least
>> require
>> > a huge API redesign which would move a lot of complexity into the user
>> > code.
>> >
>> > The only reason the combination of frame threads and HWAccel was
>> > considered useful is to allow a seamless fallback to multi-threaded
>> > software decoding if the HWAccel is not available, however the issues
>> > outlined above far outweight this.
>> >
>> > The proper solution for a fallback is to re-open the AVCodecContext with
>> > threading enabled if the HWAccel failed, which is a practice commonly
>> used
>> > by various user applications using avcodec today already.
>> >
>> > Reviewed-by: Gwenole Beauchesne <gb.devel at gmail.com>
>> > Reviewed-by: wm4 <nfxjfg at googlemail.com>
>> > Signed-off-by: Hendrik Leppkes <h.leppkes at gmail.com>
>> > ---
>> >  libavcodec/utils.c | 6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> > index 83a2078..6a58056 100644
>> > --- a/libavcodec/utils.c
>> > +++ b/libavcodec/utils.c
>> > @@ -1007,6 +1007,12 @@ static int setup_hwaccel(AVCodecContext *avctx,
>> >      AVHWAccel *hwa = find_hwaccel(avctx->codec_id, fmt);
>> >      int ret        = 0;
>> >
>> > +    if (avctx->active_thread_type & FF_THREAD_FRAME) {
>> > +        av_log(avctx, AV_LOG_ERROR,
>> > +               "Hardware accelerated decoding with frame threading is
>> not supported.\n");
>> > +        return AVERROR(EINVAL);
>> > +    }
>> > +
>> >      if (!hwa) {
>> >          av_log(avctx, AV_LOG_ERROR,
>> >                 "Could not find an AVHWAccel for the pixel format: %s",
>> > --
>> > 2.6.2.windows.1
>> >
>>
>> Last call for tangible feedback, otherwise its going in tomorrow!
>
>
> This is certainly not irreversible in case something improves, so I'd just
> commit it already. lgtm.
>

Applied.