| Message ID | 169c1509-0128-5e3d-1949-cd65f7a7aad0@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 28 Aug 2018, Gyan Doshi wrote: > > With some regularity, we have users trying to update input files using > ffmpeg, usually for the purposes of tagging, but occasionally for changing > the encoding or something else. Other apps like mp4box or taggers edit the > files in-place i.e. destination file is same as the source. FFmpeg cannot do > this. But since these users don't realize that, they will answer Yes to the > overwrite prompt and then discover their source has been destroyed. > > Attached patch checks the URL for file protocol outputs against inputs and > aborts upon a match. An option is provided for the user to force the > operation. > > The check isn't robust. In particular, it looks for exact url string matches, > so a command like > > ffmpeg -i file:somefile -some_op somefile > > will still pass through. But I consider such invocations rare. Most times > I've seen users trying this (on Stackexchange or other support forums), the > command is typically of the form, > > for i; do ffmpeg -i $i -some_op -y $i > > Such a scenario was filed as a bug in #4655 but it was marked as wontfix > since some protocols/services can manage bidir ops to the same endpoint. For > that reason, everything other than file protocol sources/sinks are exempt > i.e. http, pipes, devices..etc. > > This patch doesn't affect the semantics of '-y' and adds the check after it. Instead of this, maybe we should add support to write lock the files when opening them for reading. Then ffmpeg can request this. That would be an useful option, and not just for unexperienced users. Regards, Marton
On Tue, Aug 28, 2018 at 08:31:51AM +0200, Marton Balint wrote: > > > On Tue, 28 Aug 2018, Gyan Doshi wrote: > > > > >With some regularity, we have users trying to update input files using > >ffmpeg, usually for the purposes of tagging, but occasionally for changing > >the encoding or something else. Other apps like mp4box or taggers edit the > >files in-place i.e. destination file is same as the source. FFmpeg cannot > >do this. But since these users don't realize that, they will answer Yes to > >the overwrite prompt and then discover their source has been destroyed. > > > >Attached patch checks the URL for file protocol outputs against inputs and > >aborts upon a match. An option is provided for the user to force the > >operation. > > > >The check isn't robust. In particular, it looks for exact url string > >matches, so a command like > > > > ffmpeg -i file:somefile -some_op somefile > > > >will still pass through. But I consider such invocations rare. Most times > >I've seen users trying this (on Stackexchange or other support forums), > >the command is typically of the form, > > > > for i; do ffmpeg -i $i -some_op -y $i > > > >Such a scenario was filed as a bug in #4655 but it was marked as wontfix > >since some protocols/services can manage bidir ops to the same endpoint. > >For that reason, everything other than file protocol sources/sinks are > >exempt i.e. http, pipes, devices..etc. > > > >This patch doesn't affect the semantics of '-y' and adds the check after it. > > Instead of this, maybe we should add support to write lock the files when > opening them for reading. Then ffmpeg can request this. That would be an > useful option, and not just for unexperienced users. depending on how this is done, it could interfere with reading a file while it is being written/appeneded to by another process [...]
On 29-08-2018 02:43 AM, Michael Niedermayer wrote: > On Tue, Aug 28, 2018 at 08:31:51AM +0200, Marton Balint wrote: >> >> Instead of this, maybe we should add support to write lock the files when >> opening them for reading. Then ffmpeg can request this. That would be an >> useful option, and not just for unexperienced users. > > depending on how this is done, it could interfere with reading a file while > it is being written/appeneded to by another process Yes, a user may use a growing MPEG-TS file as input. Also, is there a universal & portable way to secure a write lock across all platforms? I think a 'soft' solution is preferable. Gyan
On 29-08-2018 09:48 AM, Gyan Doshi wrote: > On 29-08-2018 02:43 AM, Michael Niedermayer wrote: >> On Tue, Aug 28, 2018 at 08:31:51AM +0200, Marton Balint wrote: > >>> >>> Instead of this, maybe we should add support to write lock the files >>> when >>> opening them for reading. Then ffmpeg can request this. That would be an >>> useful option, and not just for unexperienced users. >> >> depending on how this is done, it could interfere with reading a file >> while >> it is being written/appeneded to by another process > > Yes, a user may use a growing MPEG-TS file as input. > > Also, is there a universal & portable way to secure a write lock across > all platforms? > > I think a 'soft' solution is preferable. Any suggestions or objections? If no, will push in a day. Gyan
On 8/30/18, Gyan Doshi <gyandoshi@gmail.com> wrote: > On 29-08-2018 09:48 AM, Gyan Doshi wrote: >> On 29-08-2018 02:43 AM, Michael Niedermayer wrote: >>> On Tue, Aug 28, 2018 at 08:31:51AM +0200, Marton Balint wrote: >> >>>> >>>> Instead of this, maybe we should add support to write lock the files >>>> when >>>> opening them for reading. Then ffmpeg can request this. That would be an >>>> useful option, and not just for unexperienced users. >>> >>> depending on how this is done, it could interfere with reading a file >>> while >>> it is being written/appeneded to by another process >> >> Yes, a user may use a growing MPEG-TS file as input. >> >> Also, is there a universal & portable way to secure a write lock across >> all platforms? >> >> I think a 'soft' solution is preferable. > > Any suggestions or objections? > > If no, will push in a day. Please do not push, until consensus is reached. What about red log warning text?
On Thu, 30 Aug 2018, Paul B Mahol wrote: > On 8/30/18, Gyan Doshi <gyandoshi@gmail.com> wrote: >> On 29-08-2018 09:48 AM, Gyan Doshi wrote: >>> On 29-08-2018 02:43 AM, Michael Niedermayer wrote: >>>> On Tue, Aug 28, 2018 at 08:31:51AM +0200, Marton Balint wrote: >>> >>>>> >>>>> Instead of this, maybe we should add support to write lock the files >>>>> when >>>>> opening them for reading. Then ffmpeg can request this. That would be an >>>>> useful option, and not just for unexperienced users. >>>> >>>> depending on how this is done, it could interfere with reading a file >>>> while >>>> it is being written/appeneded to by another process >>> >>> Yes, a user may use a growing MPEG-TS file as input. >>> >>> Also, is there a universal & portable way to secure a write lock across >>> all platforms? >>> >>> I think a 'soft' solution is preferable. >> >> Any suggestions or objections? >> >> If no, will push in a day. > > Please do not push, until consensus is reached. > > What about red log warning text? Is there any real use case when same source and destination works, so the option can be used? If not, then just make ffmpeg fail, like the cp command fails for same source and destination. I am against adding an option if it has no known use. Thanks, Marton
On 31-08-2018 04:28 AM, Marton Balint wrote: > > Is there any real use case when same source and destination works, so > the option can be used? > > If not, then just make ffmpeg fail, like the cp command fails for same > source and destination. I am against adding an option if it has no known > use. Via the file protocol, not that I know of. Will remove. Gyan
diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 3717f22d42..6fb359eabe 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -445,6 +445,12 @@ Overwrite output files without asking. Do not overwrite output files, and exit immediately if a specified output file already exists. +@item -write_to_input_file (@emph{global}) +Allow writing to an input file. Normally, this will destroy the input file and +the conversion will fail, as FFmpeg does not perform in-place editing. But some +protocols or storage services can accommodate this use-case. Verify before setting it. +This option does not override the generic output overwrite check. Disabled by default. + @item -stream_loop @var{number} (@emph{input}) Set number of times input stream shall be looped. Loop 0 means no loop, loop -1 means infinite loop. diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h index eb1eaf6363..6c1f87c7ad 100644 --- a/fftools/ffmpeg.h +++ b/fftools/ffmpeg.h @@ -606,6 +606,7 @@ extern int frame_bits_per_raw_sample; extern AVIOContext *progress_avio; extern float max_error_rate; extern char *videotoolbox_pixfmt; +extern int write_to_input_file; extern int filter_nbthreads; extern int filter_complex_nbthreads; diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index 58ec13e5a8..74e9b94bf5 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -121,6 +121,7 @@ static int input_stream_potentially_available = 0; static int ignore_unknown_streams = 0; static int copy_unknown_streams = 0; static int find_stream_info = 1; +extern int write_to_input_file = 0; static void uninit_options(OptionsContext *o) { @@ -900,13 +901,14 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic) static void assert_file_overwrite(const char *filename) { + const char *proto_name = avio_find_protocol_name(filename); + if (file_overwrite && no_file_overwrite) { fprintf(stderr, "Error, both -y and -n supplied. Exiting.\n"); exit_program(1); } if (!file_overwrite) { - const char *proto_name = avio_find_protocol_name(filename); if (proto_name && !strcmp(proto_name, "file") && avio_check(filename, 0) == 0) { if (stdin_interaction && !no_file_overwrite) { fprintf(stderr,"File '%s' already exists. Overwrite ? [y/N] ", filename); @@ -925,6 +927,20 @@ static void assert_file_overwrite(const char *filename) } } } + + if (!write_to_input_file && proto_name && !strcmp(proto_name, "file")) { + for (int i = 0; i < nb_input_files; i++) { + InputFile *file = input_files[i]; + if (file->ctx->iformat->flags & AVFMT_NOFILE) + continue; + if (!strcmp(filename, file->ctx->url)) { + av_log(NULL, AV_LOG_FATAL, "Output same as Input #%d - exiting\n", i); + av_log(NULL, AV_LOG_WARNING, "FFmpeg cannot edit existing files in-place.\n" + "Add -write_to_input_file if this is a safe operation and intended.\n"); + exit_program(1); + } + } + } } static void dump_attachment(AVStream *st, const char *filename) @@ -3315,6 +3331,8 @@ const OptionDef options[] = { "overwrite output files" }, { "n", OPT_BOOL, { &no_file_overwrite }, "never overwrite output files" }, + { "write_to_input_file", OPT_BOOL, { &write_to_input_file }, + "allow writing to an input file - normally destructive to input files and conversion may fail" }, { "ignore_unknown", OPT_BOOL, { &ignore_unknown_streams }, "Ignore unknown stream types" }, { "copy_unknown", OPT_BOOL | OPT_EXPERT, { ©_unknown_streams },
With some regularity, we have users trying to update input files using ffmpeg, usually for the purposes of tagging, but occasionally for changing the encoding or something else. Other apps like mp4box or taggers edit the files in-place i.e. destination file is same as the source. FFmpeg cannot do this. But since these users don't realize that, they will answer Yes to the overwrite prompt and then discover their source has been destroyed. Attached patch checks the URL for file protocol outputs against inputs and aborts upon a match. An option is provided for the user to force the operation. The check isn't robust. In particular, it looks for exact url string matches, so a command like ffmpeg -i file:somefile -some_op somefile will still pass through. But I consider such invocations rare. Most times I've seen users trying this (on Stackexchange or other support forums), the command is typically of the form, for i; do ffmpeg -i $i -some_op -y $i Such a scenario was filed as a bug in #4655 but it was marked as wontfix since some protocols/services can manage bidir ops to the same endpoint. For that reason, everything other than file protocol sources/sinks are exempt i.e. http, pipes, devices..etc. This patch doesn't affect the semantics of '-y' and adds the check after it. Thanks, Gyan From deb97d9b5dd1f50a7e6b560c77b81b9cd707b7c1 Mon Sep 17 00:00:00 2001 From: Gyan Doshi <ffmpeg@gyani.pro> Date: Sun, 26 Aug 2018 11:22:50 +0530 Subject: [PATCH] ffmpeg: block output == input for files Add option write_to_input_file to allow it. Fixes #4655 --- doc/ffmpeg.texi | 6 ++++++ fftools/ffmpeg.h | 1 + fftools/ffmpeg_opt.c | 20 +++++++++++++++++++- 3 files changed, 26 insertions(+), 1 deletion(-)