[FFmpeg-devel] [FFmpeg-cvslog] avfilter: add vmafmotion filter

[Michael Niedermayer]

On Sat, Oct 07, 2017 at 12:12:15PM +0200, Michael Niedermayer wrote:
> On Sat, Oct 07, 2017 at 10:09:45AM +0200, Paul B Mahol wrote:
> > On 10/7/17, James Almer <jamrial at gmail.com> wrote:
> > > On 10/6/2017 7:09 PM, wm4 wrote:
> > >> On Fri, 6 Oct 2017 18:02:44 -0300
> > >> James Almer <jamrial at gmail.com> wrote:
> > >>
> > >>> On 10/6/2017 5:44 PM, Paul B Mahol wrote:
> > >>>> On 10/6/17, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >>>>> On Fri, Oct 06, 2017 at 10:03:16AM -0400, Ronald S. Bultje wrote:
> > >>>>>> Hi,
> > >>>>>>
> > >>>>>> On Thu, Oct 5, 2017 at 7:52 PM, Michael Niedermayer
> > >>>>>> <michael at niedermayer.cc>
> > >>>>>> wrote:
> > >>>>>>
> > >>>>>>> On Sat, Sep 30, 2017 at 03:51:41PM +0000, Ashish Singh wrote:
> > >>>>>>>> ffmpeg | branch: master | Ashish Singh <ashk43712 at gmail.com> | Sat
> > >>>>>>>> Sep
> > >>>>>>> 16 02:35:58 2017 +0530| [148c8e88c43cfbabd6aee9f01ef30942cee9d359] |
> > >>>>>>> committer: Ronald S. Bultje
> > >>>>>>>>
> > >>>>>>>> avfilter: add vmafmotion filter
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Ashish Singh <ashk43712 at gmail.com>
> > >>>>>>>> Signed-off-by: Ronald S. Bultje <rsbultje at gmail.com>
> > >>>>>>>>
> > >>>>>>>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=
> > >>>>>>> 148c8e88c43cfbabd6aee9f01ef30942cee9d359
> > >>>>>>>> ---
> > >>>>>>>>
> > >>>>>>>>  Changelog                   |   1 +
> > >>>>>>>>  doc/filters.texi            |  14 ++
> > >>>>>>>>  libavfilter/Makefile        |   1 +
> > >>>>>>>>  libavfilter/allfilters.c    |   1 +
> > >>>>>>>>  libavfilter/vf_vmafmotion.c | 365 ++++++++++++++++++++++++++++++
> > >>>>>>> ++++++++++++++
> > >>>>>>>>  libavfilter/vmaf_motion.h   |  58 +++++++
> > >>>>>>>>  6 files changed, 440 insertions(+)
> > >>>>>>> [...]
> > >>>>>>>> +static av_cold int init(AVFilterContext *ctx)
> > >>>>>>>> +{
> > >>>>>>>> +    VMAFMotionContext *s = ctx->priv;
> > >>>>>>>> +
> > >>>>>>>> +    if (s->stats_file_str) {
> > >>>>>>>> +        if (!strcmp(s->stats_file_str, "-")) {
> > >>>>>>>
> > >>>>>>>> +            s->stats_file = stdout;
> > >>>>>>>
> > >>>>>>> Using stdout can interfere with the user application using the filter
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> +        } else {
> > >>>>>>>
> > >>>>>>>> +            s->stats_file = fopen(s->stats_file_str, "w");
> > >>>>>>>
> > >>>>>>> Opening a filter parameter provided string for writing is a dangerous
> > >>>>>>> way to output data. It allows one with access to the parameters to
> > >>>>>>> overwrite any writable file
> > >>>>>>>
> > >>>>>>> data should only be output in a safe way
> > >>>>>>>
> > >>>>>>
> > >>>>>> The same mechanism is present in ssim/psnr filters. I'm open to any
> > >>>>>> alternative method you suggest. These are only settable using explicit
> > >>>>>> user
> > >>>>>> interaction (and are disabled by default) so I don't particularly see
> > >>>>>> the
> > >>>>>> problem.
> > >>>>>
> > >>>>> With this a filter graph can never be taken from an untrusted source
> > >>>>>
> > >>>>> One filter that outputs statistics without writing to a user specified
> > >>>>> filename is libavfilter/af_astats.c
> > >>>>
> > >>>> So what? Get over it.
> > >>>
> > >>> What kind of reply is this? What made you think it's justified?
> > >>> He literally gave an example of an alternative method as Ronald
> > >>> requested.
> > >>>
> > >>> Some of you people need to chill out already when discussing patches.
> > >>
> > >> Michael apparently just comes up randomly with this stuff to block
> > >> patches...
> > >
> > > Nobody comes up "randomly" with concerns. Especially not to block
> > > patches for the sake of blocking, because if they were bullshit
> > > arguments then they would be easy to counter and discard.
> > >
> > > At this point you're just expressing frustration over having negative
> > > reviews or blocking concerns on patches. Sure, it would be great to
> > > finish writing something, sending it to the ML and always get a LGTM as
> > > reply, but if it doesn't happen then you shouldn't chalk it up to
> > > pedantry from the other side.
> > >
> > > So again, chill out, and discuss/argue. Ask for other devs to chime in
> > > if necessary to tip the scales. But please, stop heating up every single
> > > thread.
> > >
> > 
> > Sorry but Michael is just trolling around. Several filters already have support
> > for writting to output file, also he does not provide solution at all,
> > he is just
> > blocking progress for without reason.
> 
> You do realize that the patch discussed here was applied long before
> my comments about the arbitrary file write access ?
> Theres nothing thats blocked by this as the code in question is
> already applied.
> 
> About a solution, I didnt write alot about solutions as iam really fine
> with any solution. There are many possible solutions ...
> The libavfilter/af_astats.c filter i mentioned outputs statistics
> via metadata or av_log as 2 alternatives to direct file write.
> There are other possibilities. Also Hendriks suggestion about
> turning metadata via some other application option into a file sounds
> pretty good to me

yet another solution (if people dislike all suggestions so far) would
be adding a flag to filters which are safe to use with untrusted
parameters. Or a flag to options which are unsafe so they are blocked
from being set from an untrusted input string

There are many possibilities ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171007/47eab134/attachment.sig>