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

[Romain Beauxis <romain.beauxis@gmail.com>]

Le jeu. 15 mai 2025 à 19:54, Michael Niedermayer <michael@niedermayer.cc> a
écrit :
>
> Hi
>
> On Fri, May 16, 2025 at 12:17:14AM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
Marton
> > > Balint
> > > Sent: Freitag, 16. Mai 2025 02:00
> > > To: FFmpeg development discussions and patches <
ffmpeg-devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint: Now,
make it a
> > > Killer-Feature!
> > >
> > >
> > >
> > > On Thu, 15 May 2025, softworkz . wrote:
> > >
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
Ramiro
> > > Polla
> > > >> Sent: Donnerstag, 15. Mai 2025 23:50
> > > >> To: ffmpeg-devel@ffmpeg.org
> > > >> Subject: Re: [FFmpeg-devel] [FFmpeg-cvslog] fftools/graphprint:
Now, make
> > > it a
> > > >> Killer-Feature!
> > > >>
> > > >> Hi,
> > > >>
> > > >> On Thu, May 15, 2025 at 11:11 PM softworkz <git@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?
> > >
> > > Because ffmpeg is not a browser opener tool, but a transcoding tool.
An
> > > argument can be made for every feature you can think of (Why not add
an
> > > option which shuts down a computer when the transcoding is done? Why
not
> > > add a playable DOOM implementation so the user will not be bored when
> > > waiting for the transcode to finish).
> > >
> > > Let's just revert this. The many ffmpeg cli frontends can open
browsers if
> > > they want.
> >
> > Many good arguments can be found for both sides.
> >
> > > Because ffmpeg is not a browser opener tool
> >
> > By all respect, this isn't one.
> >
> >
> > Anyway, I will let the TC decide about this, then.
>
> Not speaking as a TC member here but IMHO
>
> 1. lets all calm down, so far we have a civil and productive discussion
>    maybe we can simply find a solution everyone is happy with!
>
> 2. all security issues must be fixed if there are some
>
> 3. there should be a configure flag to enable/disable the browser opening
feature
>    if it remains
>
> 4. can system() be replaced by fork()+exec*() ? is that something people
would
>    prefer ?

Forked processes inherit opened file descriptors unless explicitly marked
as cloexec, this is another big can of worms..

> 5. this is a cool feature, i would use this if its available, that said
>    if i had to manually open a browser with a given URL that would work
>    for me too.

At the very least there should be a prompt asking the user to confirm
what's about to happen.

A user-friend way and also secure way could be to output something that is
readily usage, for instance:
bash -c `ffmpeg <options>`

> 6. everyone who unconditionally hates this, please try it
>
> 7. as long as the discussion is nice and productive i think the TC is not
the
>    best path forward. If things degenerate into some mess then pas it to
teh
>    TC, yes
>
> 8. I think one part people should agree on first is if this should be
disabled
>    while it is being discussed (maybe a disabled by default configure
switch
>    would work?)
>
> Anyway, iam a little sleepy maybe i have some better view after sleeping
over
> this :)
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Dictatorship: All citizens are under surveillance, all their steps and
> actions recorded, for the politicians to enforce control.
> Democracy: All politicians are under surveillance, all their steps and
> actions recorded, for the citizens to enforce control.
> _______________________________________________
> 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".
_______________________________________________
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".