diff mbox

[FFmpeg-devel] avutil/file_open: log temp filename

Message ID 5be542d8-c4d1-6c3d-1c02-1a9cd039a4e5@gyani.pro
State New
Headers show

Commit Message

Gyan Doshi May 22, 2019, 7:10 a.m. UTC
Helps users to identify temp files for a given instance.

In the longer term, we should aim to clean up all temp files.

Gyan
From df7ff4b80ef424922236ac991b4664533e40b678 Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Wed, 22 May 2019 12:34:19 +0530
Subject: [PATCH] avutil/file_open: log temp filename

Helps users to identify temp files for cleanup.
---
 libavutil/file_open.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Liu Steven May 22, 2019, 8:11 a.m. UTC | #1
> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
> 
> 
> Helps users to identify temp files for a given instance.
> 
> In the longer term, we should aim to clean up all temp files.
> 
> Gyan
> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?


Thanks
Gyan Doshi May 22, 2019, 8:52 a.m. UTC | #2
On 22-05-2019 01:41 PM, Liu Steven wrote:
>
>> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
>>
>>
>> Helps users to identify temp files for a given instance.
>>
>> In the longer term, we should aim to clean up all temp files.
>>
>> Gyan
>> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
> Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
>
DEBUG and TRACE are meant to record micro-ops for debugging purposes. 
This message will be present a handful of times in the log and is 
essential for end-users so they can delete these files, which ffmpeg 
won't do, but should since these are meant to be temporary files.

Gyan
Hendrik Leppkes May 22, 2019, 8:59 a.m. UTC | #3
On Wed, May 22, 2019 at 10:53 AM Gyan <ffmpeg@gyani.pro> wrote:
>
>
>
> On 22-05-2019 01:41 PM, Liu Steven wrote:
> >
> >> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
> >>
> >>
> >> Helps users to identify temp files for a given instance.
> >>
> >> In the longer term, we should aim to clean up all temp files.
> >>
> >> Gyan
> >> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
> > Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
> >
> DEBUG and TRACE are meant to record micro-ops for debugging purposes.
> This message will be present a handful of times in the log and is
> essential for end-users so they can delete these files, which ffmpeg
> won't do, but should since these are meant to be temporary files.
>

Why don't we work on fixing this instead, as users will not know what
implications this message has in any case and just consider it spam.

- Hendrik
Steven Liu May 22, 2019, 9:19 a.m. UTC | #4
Hendrik Leppkes <h.leppkes@gmail.com> 于2019年5月22日周三 下午5:05写道:
>
> On Wed, May 22, 2019 at 10:53 AM Gyan <ffmpeg@gyani.pro> wrote:
> >
> >
> >
> > On 22-05-2019 01:41 PM, Liu Steven wrote:
> > >
> > >> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
> > >>
> > >>
> > >> Helps users to identify temp files for a given instance.
> > >>
> > >> In the longer term, we should aim to clean up all temp files.
> > >>
> > >> Gyan
> > >> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
> > > Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
> > >
> > DEBUG and TRACE are meant to record micro-ops for debugging purposes.
> > This message will be present a handful of times in the log and is
> > essential for end-users so they can delete these files, which ffmpeg
> > won't do, but should since these are meant to be temporary files.
> >
>
> Why don't we work on fixing this instead, as users will not know what
> implications this message has in any case and just consider it spam.
+1
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Gyan Doshi May 22, 2019, 9:50 a.m. UTC | #5
On 22-05-2019 02:29 PM, Hendrik Leppkes wrote:
> On Wed, May 22, 2019 at 10:53 AM Gyan <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 22-05-2019 01:41 PM, Liu Steven wrote:
>>>> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
>>>>
>>>>
>>>> Helps users to identify temp files for a given instance.
>>>>
>>>> In the longer term, we should aim to clean up all temp files.
>>>>
>>>> Gyan
>>>> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
>>> Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
>>>
>> DEBUG and TRACE are meant to record micro-ops for debugging purposes.
>> This message will be present a handful of times in the log and is
>> essential for end-users so they can delete these files, which ffmpeg
>> won't do, but should since these are meant to be temporary files.
>>
> Why don't we work on fixing this instead, as users will not know what
> implications this message has in any case and just consider it spam.

1) A grep of the tree shows only two components make use of this 
function. The cache protocol, which needs to be expressly invoked by the 
user, and the xvid encoder in 2-pass mode, which also has to be 
expressly invoked. No unsuspecting user will be presented with this message.

2) Fixing this is the ideal solution. The above two components already 
unlink the file after use, but with cache:, the file remains available 
after ffmpeg has exited. So this has to be looked at by someone 
acquainted and able to test with all the environments that ffmpeg can be 
run on. I don't have that knowledge or access. Till then, let's not the 
perfect be the enemy of the good.

Gyan
Hendrik Leppkes May 22, 2019, 9:59 a.m. UTC | #6
On Wed, May 22, 2019 at 11:51 AM Gyan <ffmpeg@gyani.pro> wrote:
>
>
>
> On 22-05-2019 02:29 PM, Hendrik Leppkes wrote:
> > On Wed, May 22, 2019 at 10:53 AM Gyan <ffmpeg@gyani.pro> wrote:
> >>
> >>
> >> On 22-05-2019 01:41 PM, Liu Steven wrote:
> >>>> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
> >>>>
> >>>>
> >>>> Helps users to identify temp files for a given instance.
> >>>>
> >>>> In the longer term, we should aim to clean up all temp files.
> >>>>
> >>>> Gyan
> >>>> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
> >>> Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
> >>>
> >> DEBUG and TRACE are meant to record micro-ops for debugging purposes.
> >> This message will be present a handful of times in the log and is
> >> essential for end-users so they can delete these files, which ffmpeg
> >> won't do, but should since these are meant to be temporary files.
> >>
> > Why don't we work on fixing this instead, as users will not know what
> > implications this message has in any case and just consider it spam.
>
> 1) A grep of the tree shows only two components make use of this
> function. The cache protocol, which needs to be expressly invoked by the
> user, and the xvid encoder in 2-pass mode, which also has to be
> expressly invoked. No unsuspecting user will be presented with this message.
>
> 2) Fixing this is the ideal solution. The above two components already
> unlink the file after use, but with cache:, the file remains available
> after ffmpeg has exited. So this has to be looked at by someone
> acquainted and able to test with all the environments that ffmpeg can be
> run on. I don't have that knowledge or access. Till then, let's not the
> perfect be the enemy of the good.
>

This message has absolutely zero indication that the user might need
to act on it, nevermind that API users might never see it in the first
place, as such I don't  see how this is even "good".

Why don't you start with collecting information which systems are even
affected by this? You must have some systems where you saw this.

- Hendrik
Gyan Doshi May 22, 2019, 10:13 a.m. UTC | #7
On 22-05-2019 03:29 PM, Hendrik Leppkes wrote:
> On Wed, May 22, 2019 at 11:51 AM Gyan <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 22-05-2019 02:29 PM, Hendrik Leppkes wrote:
>>> On Wed, May 22, 2019 at 10:53 AM Gyan <ffmpeg@gyani.pro> wrote:
>>>>
>>>> On 22-05-2019 01:41 PM, Liu Steven wrote:
>>>>>> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
>>>>>>
>>>>>>
>>>>>> Helps users to identify temp files for a given instance.
>>>>>>
>>>>>> In the longer term, we should aim to clean up all temp files.
>>>>>>
>>>>>> Gyan
>>>>>> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
>>>>> Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
>>>>>
>>>> DEBUG and TRACE are meant to record micro-ops for debugging purposes.
>>>> This message will be present a handful of times in the log and is
>>>> essential for end-users so they can delete these files, which ffmpeg
>>>> won't do, but should since these are meant to be temporary files.
>>>>
>>> Why don't we work on fixing this instead, as users will not know what
>>> implications this message has in any case and just consider it spam.
>> 1) A grep of the tree shows only two components make use of this
>> function. The cache protocol, which needs to be expressly invoked by the
>> user, and the xvid encoder in 2-pass mode, which also has to be
>> expressly invoked. No unsuspecting user will be presented with this message.
>>
>> 2) Fixing this is the ideal solution. The above two components already
>> unlink the file after use, but with cache:, the file remains available
>> after ffmpeg has exited. So this has to be looked at by someone
>> acquainted and able to test with all the environments that ffmpeg can be
>> run on. I don't have that knowledge or access. Till then, let's not the
>> perfect be the enemy of the good.
>>
> This message has absolutely zero indication that the user might need
> to act on it, nevermind that API users might never see it in the first
> place, as such I don't  see how this is even "good".
>
> Why don't you start with collecting information which systems are even
> affected by this? You must have some systems where you saw this.
This patch was prompted by a Windows user who wanted this info. As 
mentioned above, I can reproduce it with cache protocol on Windows. 
Don't have access to other systems, except one linux box.

I just saw that we do have a file delete function. Will first see if 
that works.

Gyan
Hendrik Leppkes May 22, 2019, 12:32 p.m. UTC | #8
On Wed, May 22, 2019 at 12:14 PM Gyan <ffmpeg@gyani.pro> wrote:
>
>
>
> On 22-05-2019 03:29 PM, Hendrik Leppkes wrote:
> > On Wed, May 22, 2019 at 11:51 AM Gyan <ffmpeg@gyani.pro> wrote:
> >>
> >>
> >> On 22-05-2019 02:29 PM, Hendrik Leppkes wrote:
> >>> On Wed, May 22, 2019 at 10:53 AM Gyan <ffmpeg@gyani.pro> wrote:
> >>>>
> >>>> On 22-05-2019 01:41 PM, Liu Steven wrote:
> >>>>>> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
> >>>>>>
> >>>>>>
> >>>>>> Helps users to identify temp files for a given instance.
> >>>>>>
> >>>>>> In the longer term, we should aim to clean up all temp files.
> >>>>>>
> >>>>>> Gyan
> >>>>>> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
> >>>>> Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
> >>>>>
> >>>> DEBUG and TRACE are meant to record micro-ops for debugging purposes.
> >>>> This message will be present a handful of times in the log and is
> >>>> essential for end-users so they can delete these files, which ffmpeg
> >>>> won't do, but should since these are meant to be temporary files.
> >>>>
> >>> Why don't we work on fixing this instead, as users will not know what
> >>> implications this message has in any case and just consider it spam.
> >> 1) A grep of the tree shows only two components make use of this
> >> function. The cache protocol, which needs to be expressly invoked by the
> >> user, and the xvid encoder in 2-pass mode, which also has to be
> >> expressly invoked. No unsuspecting user will be presented with this message.
> >>
> >> 2) Fixing this is the ideal solution. The above two components already
> >> unlink the file after use, but with cache:, the file remains available
> >> after ffmpeg has exited. So this has to be looked at by someone
> >> acquainted and able to test with all the environments that ffmpeg can be
> >> run on. I don't have that knowledge or access. Till then, let's not the
> >> perfect be the enemy of the good.
> >>
> > This message has absolutely zero indication that the user might need
> > to act on it, nevermind that API users might never see it in the first
> > place, as such I don't  see how this is even "good".
> >
> > Why don't you start with collecting information which systems are even
> > affected by this? You must have some systems where you saw this.
> This patch was prompted by a Windows user who wanted this info. As
> mentioned above, I can reproduce it with cache protocol on Windows.
> Don't have access to other systems, except one linux box.
>
> I just saw that we do have a file delete function. Will first see if
> that works.
>

I know that it doesn't work on Windows, because you have to
specifically request that a file can be marked for deletion while its
still open, which we do not do.

I'm working on a patch to see if we can fix this.

- Hendrik
Gyan Doshi May 22, 2019, 12:34 p.m. UTC | #9
On 22-05-2019 06:02 PM, Hendrik Leppkes wrote:
> On Wed, May 22, 2019 at 12:14 PM Gyan <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 22-05-2019 03:29 PM, Hendrik Leppkes wrote:
>>> On Wed, May 22, 2019 at 11:51 AM Gyan <ffmpeg@gyani.pro> wrote:
>>>>
>>>> On 22-05-2019 02:29 PM, Hendrik Leppkes wrote:
>>>>> On Wed, May 22, 2019 at 10:53 AM Gyan <ffmpeg@gyani.pro> wrote:
>>>>>> On 22-05-2019 01:41 PM, Liu Steven wrote:
>>>>>>>> 在 2019年5月22日,下午3:10,Gyan <ffmpeg@gyani.pro> 写道:
>>>>>>>>
>>>>>>>>
>>>>>>>> Helps users to identify temp files for a given instance.
>>>>>>>>
>>>>>>>> In the longer term, we should aim to clean up all temp files.
>>>>>>>>
>>>>>>>> Gyan
>>>>>>>> <0001-avutil-file_open-log-temp-filename.patch>_______________________________________________
>>>>>>> Why don’t use AV_LOG_DEBUG or AV_LOG_TRACE?
>>>>>>>
>>>>>> DEBUG and TRACE are meant to record micro-ops for debugging purposes.
>>>>>> This message will be present a handful of times in the log and is
>>>>>> essential for end-users so they can delete these files, which ffmpeg
>>>>>> won't do, but should since these are meant to be temporary files.
>>>>>>
>>>>> Why don't we work on fixing this instead, as users will not know what
>>>>> implications this message has in any case and just consider it spam.
>>>> 1) A grep of the tree shows only two components make use of this
>>>> function. The cache protocol, which needs to be expressly invoked by the
>>>> user, and the xvid encoder in 2-pass mode, which also has to be
>>>> expressly invoked. No unsuspecting user will be presented with this message.
>>>>
>>>> 2) Fixing this is the ideal solution. The above two components already
>>>> unlink the file after use, but with cache:, the file remains available
>>>> after ffmpeg has exited. So this has to be looked at by someone
>>>> acquainted and able to test with all the environments that ffmpeg can be
>>>> run on. I don't have that knowledge or access. Till then, let's not the
>>>> perfect be the enemy of the good.
>>>>
>>> This message has absolutely zero indication that the user might need
>>> to act on it, nevermind that API users might never see it in the first
>>> place, as such I don't  see how this is even "good".
>>>
>>> Why don't you start with collecting information which systems are even
>>> affected by this? You must have some systems where you saw this.
>> This patch was prompted by a Windows user who wanted this info. As
>> mentioned above, I can reproduce it with cache protocol on Windows.
>> Don't have access to other systems, except one linux box.
>>
>> I just saw that we do have a file delete function. Will first see if
>> that works.
>>
> I know that it doesn't work on Windows, because you have to
> specifically request that a file can be marked for deletion while its
> still open, which we do not do.
>
> I'm working on a patch to see if we can fix this.
I've fixed it locally for the cache protocol. Sending it soon.

Gyan
diff mbox

Patch

diff --git a/libavutil/file_open.c b/libavutil/file_open.c
index cc302f2f76..75d1c8befd 100644
--- a/libavutil/file_open.c
+++ b/libavutil/file_open.c
@@ -152,6 +152,8 @@  int avpriv_tempfile(const char *prefix, char **filename, int log_offset, void *l
         av_freep(filename);
         return err;
     }
+    av_log(&file_log_ctx, AV_LOG_INFO, "Opened temporary file %s\n", *filename);
+
     return fd; /* success */
 }