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

[Marton Balint]

On Fri, 16 May 2025, Michael Niedermayer wrote:

> Hi
>
> On Fri, May 16, 2025 at 12:17:14AM +0000, softworkz . wrote:
>>
>>
>>> -----Original Message-----
>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Marton
>>> Balint
>>> Sent: Freitag, 16. Mai 2025 02:00
>>> 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 Thu, 15 May 2025, softworkz . wrote:
>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Ramiro
>>> Polla
>>>>> Sent: Donnerstag, 15. Mai 2025 23:50
>>>>> To: ffmpeg-devel at 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 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?
>>>
>>> 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

Disagree. We should not clutter our code with this feature, it is out of 
our scope.

>
> 4. can system() be replaced by fork()+exec*() ? is that something people would
>   prefer ?

No.

>
> 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.

Exactly.

>
> 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?)

Yes. Vast majority of the people dislike this.

Regards,
Marton