[FFmpeg-devel] [PATCH] avcodec: only warn about hwaccel with frame threads

[Ronald S. Bultje]

Hi,

On Sun, Jan 24, 2016 at 6:49 PM, Hendrik Leppkes <h.leppkes at gmail.com>
wrote:

> On Sat, Jan 23, 2016 at 4:55 PM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> > Hi,
> >
> > On Sat, Jan 23, 2016 at 8:51 AM, Ronald S. Bultje <rsbultje at gmail.com>
> > wrote:
> >
> >> Hi,
> >>
> >> On Sat, Jan 23, 2016 at 8:45 AM, Hendrik Leppkes <h.leppkes at gmail.com>
> >> wrote:
> >>
> >>> On Sat, Jan 23, 2016 at 2:38 PM, Ronald S. Bultje <rsbultje at gmail.com>
> >>> wrote:
> >>> > Hi,
> >>> >
> >>> > On Sat, Jan 23, 2016 at 8:28 AM, wm4 <nfxjfg at googlemail.com> wrote:
> >>> >
> >>> >> On Sat, 23 Jan 2016 13:50:32 +0100
> >>> >> Michael Niedermayer <michael at niedermayer.cc> wrote:
> >>> >>
> >>> >> > On Sat, Jan 23, 2016 at 11:42:54AM +0100, Hendrik Leppkes wrote:
> >>> >> > > On Sat, Jan 23, 2016 at 10:12 AM, Andreas Cadhalpun
> >>> >> > > <andreas.cadhalpun at googlemail.com> wrote:
> >>> >> > > > VLC uses hwaccel with frame threads and it works fine, but
> >>> returning
> >>> >> > > > an error here made it fail.
> >>> >> > > >
> >>> >> > > > This regression was introduced in commit 31741ae.
> >>> >> > > >
> >>> >> > >
> >>> >> > > I'm still opposed to this, and so is everyone else that
> commented
> >>> on
> >>> >> the issue.
> >>> >> >
> >>> >> > I have no oppinion on the patch itself but i think
> >>> >> > if MT+HWaccel works in some case(s) and is faster then there
> should
> >>> be
> >>> >> > a way to enable that. We have optional "fast" decoding support too
> >>> >> > that skips some checks and could crash or uses simpler
> dequantization
> >>> >> > or our skip loopfilter support, ...
> >>> >>
> >>> >> Whether it's faster is an entirely different question.
> >>> >>
> >>> >> The problem is that we don't want to enable hwaccel with MT because
> >>> >> it's much pain for little gain, but then if fallback to software
> >>> >> happens, decoding will remain single-threaded.
> >>> >>
> >>> >> Possible solutions:
> >>> >> - somehow allow changing the number of frame threading threads
> during
> >>> >>   fallback
> >>> >> - add a wrapper codec, which behaves like the old one, but reopens
> the
> >>> >>   codec behind the scenes in order to change the number of threads
> on
> >>> >>   fallback
> >>> >> - making sure hwaccels actually work with MT (complicated => not
> worth
> >>> >>   the trouble)
> >>> >
> >>> >
> >>> > Why don't we ignore threading (set active_thread_type to NONE) if
> >>> hwaccel
> >>> > is enabled? Doesn't that fix everything in one single place?
> >>> >
> >>>
> >>> Threading is already setup and running when the hwaccel gets
> >>> activated, and its not designed to go in and out of threading mode at
> >>> will.
> >>
> >>
> >> Ok, so we can change how threading works. A fairly simple solution may
> be
> >> to have a hwaccel flag that says whether it's threadsafe (whitelist
> style),
> >> and if that's not set and hwaccel is enabled and active, have either
> >> hwaccel or threading code "throttle" the number of iterating threads to
> 1
> >> (i.e. you have N running threads, but N-1 idle and only 1 is ever
> accessed
> >> while throttle-mode is active). This way, software fallback still works.
> >>
> >
> > I'd want to poke this again before it gets lost. I'd like software
> fallback
> > with threading to work, and hwaccel to be used by default (w/o threading,
> > if needed). This proposal still seems the only one that accomplishes
> that.
> > Can I get votes on this? I can implement if I get access to systems with
> > hardware support (or examples for local reproduction).
> >
>
> Fully functional switch between hwaccel and threading would of course
> also be nice.
>
> Because of thread safety issues, the MT code was already "throttled"
> to never execute more than one decoding thread at a time with hwaccel.
> It does this by keeping the thread in "setup" mode until its done
> executing, so any later threads do not get executed yet.
>
> (This is why I don't see how hwaccel+MT could yield significant
> performance gains)
>
> The concept in itself isn't too bad, and it avoided the majority of
> problems with thread safety and hwaccels.
> However, here is the problem: The threads still execute async, which
> means the user code has no way to sync to them and avoid calling
> hardware APIs at the same time as the decode thread.
>
> A start would be that the threads would be further synchronized so
> that threads only run while avcodec_decode_video2 is executed, and no
> longer run once that function returns, so that the user code can be
> sure to not run into thread safety issues.
> This solves the main problem of image corruption due to parallel use
> of image surfaces, and the rare crashes.
>

This is what I was proposing, yes.


> Unfortunately that doesn't alleviate the other issues, like the
> complexity needed in the decoders during frame threading, or the extra
> resources needed (extra image surfaces for every thread).
>

So, the extra code is just in the decoders, which already need it anyway
(because they implement frame-mt), right? Or do hwaccels need extra code
also?

The extra resources aren't a big deal IMO. Memory use isn't typically a big
issue, we're adding a fw kb extra for contexts but practically all memory
is in framebuffers regardless.


> What I would really *love* to see here would be the ability to
> entirely and completely switch in and out of threading mode at will.
> If hwaccel turns on, terminate the worker threads, sync their contexts
> back to the main context, and proceed with single threaded decoding.
> And vice-versa when the hwaccel goes away for some reason.
> Is this full switch feasible? Its certainly not trivial and may have a
> few roadblocks along the way. But maybe it could be done. Having a m:n
> decode  API would certainly make it easier, however.


m:n would help in a lot of ways, yes. But until that and some more issues
are solved, I don't think real switching is very realistic...

It's be nice in theory, though. Eventually...

Ronald