| Message ID | 20181016121608.19072-1-robux4@ycbcr.xyz |
|---|---|
| State | New |
| Headers | show |
On 10/16/2018 9:16 AM, Steve Lhomme wrote: > If the decoder provides its own buffers it might not be able to release its > buffers once it has been closed. (this is the case with dav1d). > --- > fftools/ffmpeg.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > index da4259a9a8..faf62475a2 100644 > --- a/fftools/ffmpeg.c > +++ b/fftools/ffmpeg.c > @@ -4738,6 +4738,7 @@ static int transcode(void) > if (ost->encoding_needed) { > av_freep(&ost->enc_ctx->stats_in); > } > + av_frame_unref(ost->last_frame); > total_packets_written += ost->packets_written; > } I'm not against this change, but this issue should be solved within dav1d as well. Either the caller fully owns the picture returned by dav1d, or it doesn't. "Partially owns it while some other context is still valid" is not really acceptable.
On Tue, Oct 16, 2018 at 4:02 PM James Almer <jamrial@gmail.com> wrote: > > On 10/16/2018 9:16 AM, Steve Lhomme wrote: > > If the decoder provides its own buffers it might not be able to release its > > buffers once it has been closed. (this is the case with dav1d). > > --- > > fftools/ffmpeg.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > > index da4259a9a8..faf62475a2 100644 > > --- a/fftools/ffmpeg.c > > +++ b/fftools/ffmpeg.c > > @@ -4738,6 +4738,7 @@ static int transcode(void) > > if (ost->encoding_needed) { > > av_freep(&ost->enc_ctx->stats_in); > > } > > + av_frame_unref(ost->last_frame); > > total_packets_written += ost->packets_written; > > } > > I'm not against this change, but this issue should be solved within > dav1d as well. Either the caller fully owns the picture returned by > dav1d, or it doesn't. "Partially owns it while some other context is > still valid" is not really acceptable. I agree. AVFrames guarantee to remain valid independently of which component they ever came out of. So if dav1d cannot provide that, either we need to solve that in the dav1d wrapper (ie. by keeping an instance to the dav1d context tied to the frame like we do for some hardware frames), or dav1d fixes that. - Hendrik
On 16/10/2018 16:59, Hendrik Leppkes wrote: > On Tue, Oct 16, 2018 at 4:02 PM James Almer <jamrial@gmail.com> wrote: >> On 10/16/2018 9:16 AM, Steve Lhomme wrote: >>> If the decoder provides its own buffers it might not be able to release its >>> buffers once it has been closed. (this is the case with dav1d). >>> --- >>> fftools/ffmpeg.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c >>> index da4259a9a8..faf62475a2 100644 >>> --- a/fftools/ffmpeg.c >>> +++ b/fftools/ffmpeg.c >>> @@ -4738,6 +4738,7 @@ static int transcode(void) >>> if (ost->encoding_needed) { >>> av_freep(&ost->enc_ctx->stats_in); >>> } >>> + av_frame_unref(ost->last_frame); >>> total_packets_written += ost->packets_written; >>> } >> I'm not against this change, but this issue should be solved within >> dav1d as well. Either the caller fully owns the picture returned by >> dav1d, or it doesn't. "Partially owns it while some other context is >> still valid" is not really acceptable. You can't ask any library to own content even after you have closed it (and potentially unloaded the DLL). > I agree. AVFrames guarantee to remain valid independently of which > component they ever came out of. So if dav1d cannot provide that, > either we need to solve that in the dav1d wrapper (ie. by keeping an > instance to the dav1d context tied to the frame like we do for some > hardware frames), or dav1d fixes that. If we provide a picture allocator to dav1d that wraps AVFrame we allocate it should not be an issue. We would own them no matter what.
On 10/16/2018 12:34 PM, Steve Lhomme wrote: > On 16/10/2018 16:59, Hendrik Leppkes wrote: >> On Tue, Oct 16, 2018 at 4:02 PM James Almer <jamrial@gmail.com> wrote: >>> On 10/16/2018 9:16 AM, Steve Lhomme wrote: >>>> If the decoder provides its own buffers it might not be able to >>>> release its >>>> buffers once it has been closed. (this is the case with dav1d). >>>> --- >>>> fftools/ffmpeg.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c >>>> index da4259a9a8..faf62475a2 100644 >>>> --- a/fftools/ffmpeg.c >>>> +++ b/fftools/ffmpeg.c >>>> @@ -4738,6 +4738,7 @@ static int transcode(void) >>>> if (ost->encoding_needed) { >>>> av_freep(&ost->enc_ctx->stats_in); >>>> } >>>> + av_frame_unref(ost->last_frame); >>>> total_packets_written += ost->packets_written; >>>> } >>> I'm not against this change, but this issue should be solved within >>> dav1d as well. Either the caller fully owns the picture returned by >>> dav1d, or it doesn't. "Partially owns it while some other context is >>> still valid" is not really acceptable. > > You can't ask any library to own content even after you have closed it > (and potentially unloaded the DLL). As i said in dav1d's merge request, with libavcodec i can open an encoder, generate an AVPacket, close the encoder, open a bitstream filter, filter the packet i got from the encoder, close the bitstream filter, and still fully own the packet without having in it any dangling pointer to some callback stored in a long dead context. Same thing with AVFrames. This patch prevents ffmpeg.c closing the decoder (and thus the Dav1dContext) before a reference to one AVFrame, owned by ffmpeg.c and supposedly standalone, was freed. That's ok, but ffmpeg.c is just one libavcodec user, and we have no control whatsoever of what other libavcodec users will do with a returned AVFrame from avcodec_receive_frame(). And as Hendrik said, they are guaranteed to remain valid regardless of the component they came out of, so this is not acceptable. > >> I agree. AVFrames guarantee to remain valid independently of which >> component they ever came out of. So if dav1d cannot provide that, >> either we need to solve that in the dav1d wrapper (ie. by keeping an >> instance to the dav1d context tied to the frame like we do for some >> hardware frames), or dav1d fixes that. > > If we provide a picture allocator to dav1d that wraps AVFrame we > allocate it should not be an issue. We would own them no matter what. Yes, but dav1d will be used by more projects than just ffmpeg. The internal allocator is going to be the most used one, and until this merge, returned pictures were not constrained by the lifetime of the decoder context that produced them. This is effectively a regression.
On 16/10/2018 18:04, James Almer wrote: > On 10/16/2018 12:34 PM, Steve Lhomme wrote: >> On 16/10/2018 16:59, Hendrik Leppkes wrote: >>> On Tue, Oct 16, 2018 at 4:02 PM James Almer <jamrial@gmail.com> wrote: >>>> On 10/16/2018 9:16 AM, Steve Lhomme wrote: >>>>> If the decoder provides its own buffers it might not be able to >>>>> release its >>>>> buffers once it has been closed. (this is the case with dav1d). >>>>> --- >>>>> fftools/ffmpeg.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c >>>>> index da4259a9a8..faf62475a2 100644 >>>>> --- a/fftools/ffmpeg.c >>>>> +++ b/fftools/ffmpeg.c >>>>> @@ -4738,6 +4738,7 @@ static int transcode(void) >>>>> if (ost->encoding_needed) { >>>>> av_freep(&ost->enc_ctx->stats_in); >>>>> } >>>>> + av_frame_unref(ost->last_frame); >>>>> total_packets_written += ost->packets_written; >>>>> } >>>> I'm not against this change, but this issue should be solved within >>>> dav1d as well. Either the caller fully owns the picture returned by >>>> dav1d, or it doesn't. "Partially owns it while some other context is >>>> still valid" is not really acceptable. >> You can't ask any library to own content even after you have closed it >> (and potentially unloaded the DLL). > As i said in dav1d's merge request, with libavcodec i can open an > encoder, generate an AVPacket, close the encoder, open a bitstream > filter, filter the packet i got from the encoder, close the bitstream > filter, and still fully own the packet without having in it any dangling > pointer to some callback stored in a long dead context. Same thing with > AVFrames. It generally works because it's usually used statically and with no external decoder, loaded dynamically. > This patch prevents ffmpeg.c closing the decoder (and thus the > Dav1dContext) before a reference to one AVFrame, owned by ffmpeg.c and > supposedly standalone, was freed. That's ok, but ffmpeg.c is just one > libavcodec user, and we have no control whatsoever of what other > libavcodec users will do with a returned AVFrame from > avcodec_receive_frame(). > And as Hendrik said, they are guaranteed to remain valid regardless of > the component they came out of, so this is not acceptable. I'd be curious to see how it works with DXVA2/D3D11VA where a DLL needs to be loaded and the buffers come for that DLL. If last_frame holds a DXVA2/D3D11 texture and tries to free it after the DLL has been unloaded it shouldn't work well.
On 17/10/2018 09:43, Steve Lhomme wrote: > On 16/10/2018 18:04, James Almer wrote: >> On 10/16/2018 12:34 PM, Steve Lhomme wrote: >>> On 16/10/2018 16:59, Hendrik Leppkes wrote: >>>> On Tue, Oct 16, 2018 at 4:02 PM James Almer <jamrial@gmail.com> wrote: >>>>> On 10/16/2018 9:16 AM, Steve Lhomme wrote: >>>>>> If the decoder provides its own buffers it might not be able to >>>>>> release its >>>>>> buffers once it has been closed. (this is the case with dav1d). >>>>>> --- >>>>>> fftools/ffmpeg.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c >>>>>> index da4259a9a8..faf62475a2 100644 >>>>>> --- a/fftools/ffmpeg.c >>>>>> +++ b/fftools/ffmpeg.c >>>>>> @@ -4738,6 +4738,7 @@ static int transcode(void) >>>>>> if (ost->encoding_needed) { >>>>>> av_freep(&ost->enc_ctx->stats_in); >>>>>> } >>>>>> + av_frame_unref(ost->last_frame); >>>>>> total_packets_written += ost->packets_written; >>>>>> } >>>>> I'm not against this change, but this issue should be solved within >>>>> dav1d as well. Either the caller fully owns the picture returned by >>>>> dav1d, or it doesn't. "Partially owns it while some other context is >>>>> still valid" is not really acceptable. >>> You can't ask any library to own content even after you have closed it >>> (and potentially unloaded the DLL). >> As i said in dav1d's merge request, with libavcodec i can open an >> encoder, generate an AVPacket, close the encoder, open a bitstream >> filter, filter the packet i got from the encoder, close the bitstream >> filter, and still fully own the packet without having in it any dangling >> pointer to some callback stored in a long dead context. Same thing with >> AVFrames. > > It generally works because it's usually used statically and with no > external decoder, loaded dynamically. > >> This patch prevents ffmpeg.c closing the decoder (and thus the >> Dav1dContext) before a reference to one AVFrame, owned by ffmpeg.c and >> supposedly standalone, was freed. That's ok, but ffmpeg.c is just one >> libavcodec user, and we have no control whatsoever of what other >> libavcodec users will do with a returned AVFrame from >> avcodec_receive_frame(). >> And as Hendrik said, they are guaranteed to remain valid regardless of >> the component they came out of, so this is not acceptable. > > I'd be curious to see how it works with DXVA2/D3D11VA where a DLL > needs to be loaded and the buffers come for that DLL. If last_frame > holds a DXVA2/D3D11 texture and tries to free it after the DLL has > been unloaded it shouldn't work well. I had a quick look. It turns out that the DLLs are loaded but never unloaded. So of course it will never execute code in unloaded DLLs.
On Wed, Oct 17, 2018 at 10:14 AM Steve Lhomme <robux4@ycbcr.xyz> wrote: > > On 17/10/2018 09:43, Steve Lhomme wrote: > > On 16/10/2018 18:04, James Almer wrote: > >> On 10/16/2018 12:34 PM, Steve Lhomme wrote: > >>> On 16/10/2018 16:59, Hendrik Leppkes wrote: > >>>> On Tue, Oct 16, 2018 at 4:02 PM James Almer <jamrial@gmail.com> wrote: > >>>>> On 10/16/2018 9:16 AM, Steve Lhomme wrote: > >>>>>> If the decoder provides its own buffers it might not be able to > >>>>>> release its > >>>>>> buffers once it has been closed. (this is the case with dav1d). > >>>>>> --- > >>>>>> fftools/ffmpeg.c | 1 + > >>>>>> 1 file changed, 1 insertion(+) > >>>>>> > >>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > >>>>>> index da4259a9a8..faf62475a2 100644 > >>>>>> --- a/fftools/ffmpeg.c > >>>>>> +++ b/fftools/ffmpeg.c > >>>>>> @@ -4738,6 +4738,7 @@ static int transcode(void) > >>>>>> if (ost->encoding_needed) { > >>>>>> av_freep(&ost->enc_ctx->stats_in); > >>>>>> } > >>>>>> + av_frame_unref(ost->last_frame); > >>>>>> total_packets_written += ost->packets_written; > >>>>>> } > >>>>> I'm not against this change, but this issue should be solved within > >>>>> dav1d as well. Either the caller fully owns the picture returned by > >>>>> dav1d, or it doesn't. "Partially owns it while some other context is > >>>>> still valid" is not really acceptable. > >>> You can't ask any library to own content even after you have closed it > >>> (and potentially unloaded the DLL). > >> As i said in dav1d's merge request, with libavcodec i can open an > >> encoder, generate an AVPacket, close the encoder, open a bitstream > >> filter, filter the packet i got from the encoder, close the bitstream > >> filter, and still fully own the packet without having in it any dangling > >> pointer to some callback stored in a long dead context. Same thing with > >> AVFrames. > > > > It generally works because it's usually used statically and with no > > external decoder, loaded dynamically. > > > >> This patch prevents ffmpeg.c closing the decoder (and thus the > >> Dav1dContext) before a reference to one AVFrame, owned by ffmpeg.c and > >> supposedly standalone, was freed. That's ok, but ffmpeg.c is just one > >> libavcodec user, and we have no control whatsoever of what other > >> libavcodec users will do with a returned AVFrame from > >> avcodec_receive_frame(). > >> And as Hendrik said, they are guaranteed to remain valid regardless of > >> the component they came out of, so this is not acceptable. > > > > I'd be curious to see how it works with DXVA2/D3D11VA where a DLL > > needs to be loaded and the buffers come for that DLL. If last_frame > > holds a DXVA2/D3D11 texture and tries to free it after the DLL has > > been unloaded it shouldn't work well. > > I had a quick look. It turns out that the DLLs are loaded but never > unloaded. So of course it will never execute code in unloaded DLLs. > Unloading DLLs, at least system DLLs on Windows, is mostly pointless anyway. For dav1d, it would be linked by the linker to avcodec, and thus you wouldn't unload it unless you unload avcodec, which you really shouldn't do unless you destroyed all objects from it anyway. Basically, DLL unloading is not really a factor to worry about much. - Hendrik
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index da4259a9a8..faf62475a2 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -4738,6 +4738,7 @@ static int transcode(void) if (ost->encoding_needed) { av_freep(&ost->enc_ctx->stats_in); } + av_frame_unref(ost->last_frame); total_packets_written += ost->packets_written; }