[FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now, make it a Killer-Feature!

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Freitag, 16. Mai 2025 00:35
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now, make it a
> Killer-Feature!
> 
> On 15/05/2025 23:19, softworkz . wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Ramiro
> Polla
> >> Sent: Freitag, 16. Mai 2025 00:13
> >> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now, make
> it a
> >> Killer-Feature!
> >>
> >> On Fri, May 16, 2025 at 12:00 AM softworkz .
> >> <softworkz-at-hotmail.com at ffmpeg.org> wrote:
> >>>> On Thu, May 15, 2025 at 11:11 PM softworkz <git at videolan.org> wrote:
> >>>> [...]
> >>>>> diff --git a/fftools/graph/filelauncher.c b/fftools/graph/filelauncher.c
> >>>>> new file mode 100644
> >>>>> index 0000000000..45514ca599
> >>>>> --- /dev/null
> >>>>> +++ b/fftools/graph/filelauncher.c
> >>>> [...]
> >>>>> +int ff_open_html_in_browser(const char *html_path)
> >>>>> +{
> >>>>> +    if (!html_path || !*html_path)
> >>>>> +        return -1;
> >>>>> +
> >>>>> +#if defined(_WIN32)
> >>>>> +
> >>>>> +    // --- Windows ---------------------------------
> >>>>> +    {
> >>>>> +        HINSTANCE rc = ShellExecuteA(NULL, "open", html_path, NULL,
> >> NULL,
> >>>> SW_SHOWNORMAL);
> >>>>> +        if ((UINT_PTR)rc <= 32) {
> >>>>> +            // Fallback: system("start ...")
> >>>>> +            char cmd[1024];
> >>>>> +            _snprintf_s(cmd, sizeof(cmd), _TRUNCATE, "start \"\"
> >> \"%s\"",
> >>>> html_path);
> >>>>> +            if (system(cmd) != 0)
> >>>>> +                return -1;
> >>>>> +        }
> >>>>> +        return 0;
> >>>>> +    }
> >>>>> +
> >>>>> +#elif defined(__APPLE__)
> >>>>> +
> >>>>> +    // --- macOS -----------------------------------
> >>>>> +    {
> >>>>> +        // "open" is the macOS command to open a file/URL with the
> >> default
> >>>> application
> >>>>> +        char cmd[1024];
> >>>>> +        snprintf(cmd, sizeof(cmd), "open '%s' 1>/dev/null 2>&1 &",
> >>>> html_path);
> >>>>> +        if (system(cmd) != 0)
> >>>>> +            return -1;
> >>>>> +        return 0;
> >>>>> +    }
> >>>>> +
> >>>>> +#else
> >>>>> +
> >>>>> +    // --- Linux / Unix-like -----------------------
> >>>>> +    // We'll try xdg-open, then gnome-open, then kfmclient
> >>>>> +    {
> >>>>> +        // Helper macro to try one browser command
> >>>>> +        // Returns 0 on success, -1 on failure
> >>>>> +        #define TRY_CMD(prog) do {                                   \
> >>>>> +            char buf[1024];                                          \
> >>>>> +            snprintf(buf, sizeof(buf), "%s '%s' 1>/dev/null 2>&1 &", \
> >>>>> +                     (prog), html_path);                              \
> >>>>> +            int ret = system(buf);                                    \
> >>>>> +            /* On Unix: system() returns -1 if the shell can't run. */\
> >>>>> +            /* Otherwise, check exit code in lower 8 bits.
> >> */\
> >>>>> +            if (ret != -1 && WIFEXITED(ret) && WEXITSTATUS(ret) == 0) \
> >>>>> +                return 0;                                             \
> >>>>> +        } while (0)
> >>>>> +
> >>>>> +        TRY_CMD("xdg-open");
> >>>>> +        TRY_CMD("gnome-open");
> >>>>> +        TRY_CMD("kfmclient exec");
> >>>>> +
> >>>>> +        fprintf(stderr, "Could not open '%s' in a browser.\n",
> >> html_path);
> >>>>> +        return -1;
> >>>>> +    }
> >>>>> +
> >>>>> +#endif
> >>>>> +}
> >>>> [...]
> >>>>
> >>>> Sorry I didn't have a closer look at the patchset while it was under
> >>>> review, but system(cmd) is a big no-no. We could create a file with an
> >>>> explicit path passed by the user, but then it's up to the user to open
> >>>> it.
> >>>
> >>> What's bad about opening a file in the browser when that's the documented
> >>> behavior of the cli parameter?
> >>
> >> Straight out of ChatGPT:
> >> I understand the motivation — making the feature more user-friendly by
> >> launching the result directly is a nice touch. The concern isn't with
> >> the feature itself, but rather with the way it's implemented.
> >> Using system() to launch a browser introduces potential security
> >> risks, especially if the file path is ever constructed from untrusted
> >> input (e.g. future scripting, API wrapping, or unexpected shell
> >> expansion). It's generally discouraged in projects like FFmpeg, where
> >> robustness and security are critical.
> >
> > Hi,
> >
> > of course I understand that.
> > But it isn't constructed from untrusted input.
> >
> > Best regards
> > sw
> 
> $ export TMPDIR="'; rm -rf / ;'\\\\"
> $ ./ffmpeg_g -sg -i /dev/null -f null -
> 
> Calls to system are just not a good idea in general.  Suggest printing the
> file name and let the user open the file however they choose to.

I just wanted to note that this is not the normal way how it works.
The normal way is that you provide the target output file on the command 
line, so that part exists already.

"-sg" is just on top of it which saves you from dealing with file names
and manual opening. Instead, you can run command after command and the
graph will open in the browser in one tab after another, so that you can
compare easily across different runs without any manual work.


Thanks
sw