[MPlayer-dev-eng] [PATCH] stream: add fallback to ffmpeg for unknown streams

[Alexander Strasser]

Hi!

On 2020-01-15 01:25 +0100, Vincenzo Nicosia wrote:
> On Tue, Jan 14, 2020 at 02:20:56PM +0100, Vincenzo Nicosia wrote:
> > Dear devs,
> >
> > please find attached a preliminary attempt to let open_stream_full
> > fallback to ffmpeg if the url does not belong to any known scheme.
> > This allows to open stuff like:
> >
> >   mplayer gopher://bitreich.org/9/memecache/firewall.webm
> >
>
> Dear mplayer devs,
>
> please find below a shorter and more elegant patch, which avoids a
> goto and allocates the fname string only if needed. Thanks to Quentin
> Rameau for comments on the previous one.

Thanks for taking a stab at this!


> Index: stream/stream.c
> ===================================================================
> --- stream/stream.c	(revision 38159)
> +++ stream/stream.c	(working copy)
> @@ -38,6 +38,7 @@
>  #endif
>
>  #include "mp_msg.h"
> +#include "mp_strings.h"
>  #include "help_mp.h"
>  #include "osdep/shmem.h"
>  #include "osdep/timer.h"
> @@ -218,7 +219,7 @@
>  }
>
>
> -stream_t* open_stream_full(const char* filename,int mode, char** options, int* file_format) {
> +stream_t* open_stream_full_attempt(const char* filename,int mode, char** options, int* file_format) {
>    int i,j;
>
>    for(i = 0 ; auto_open_streams[i] ; i++) {
> @@ -256,6 +257,22 @@
>    return NULL;
>  }
>
> +stream_t* open_stream_full(const char* filename,int mode, char** options, int* file_format) {
> +
> +  char *fname;

Should probably be moved inside the if-body scope.


> +  stream_t *res=open_stream_full_attempt(filename, mode, options, file_format);

I'm not so happy with the identifier names

* fname (maybe ffmpeg_url)
* open_stream_full_attempt (no better idea ATM)


> +#ifdef CONFIG_FFMPEG
> +  if (res == NULL){
> +  /* If everything else failed, and ffmpeg is enabled, then make a last try via ffmpeg */

Comment not needed IMHO. There is also the mp_msg below which gives
similar information.


> +    fname=mp_asprintf("ffmpeg://%s",filename);
> +    mp_msg(MSGT_OPEN,MSGL_WARN, "No stream plugin matched. Trying via ffmpeg now\n");
> +    res=open_stream_full_attempt(fname, mode, options, file_format);

> +    free(fname);

Calling free here should be OK, because the string is duplicated before
being passed into the stream plugin's open function.


> +#endif
> +  }

This won't compile if CONFIG_FFMPEG is not defined.

> +  return res;
> +}
> +
>  stream_t* open_output_stream(const char* filename, char** options) {
>    int file_format; //unused
>    if(!filename) {

*Don't care much about the cosmetic advice I have given at this point.*
If we go with this approach we can get the cosmetics right just before
committing...

It builds and works for me, but I'm not sure about the exact approach.

The code will now also retry for every typoed filename to open with
ffmpeg://typo_here . While that might not be as dangerous as it looks
in the first place (not yet sure about it), it definitely adds noise
by printing the messages and especially with the rewritten filename
it might confuse users. Like why is mplayer trying to ffmpeg:// my
local video file or dvd.

If we still decide to go with this approach, another thing to think
about is what happens if the open_stream_full function is used to
open an output stream. Do we want the ffmpeg:// extra try for those
usages too?

Maybe we should think about constraining the last resort ffmpeg://
try more to usage with protocols specified with name:// . Maybe we
could even test beforehand if the ffmpeg at hand has support for the
given protocol if that's feasible.

No more better ideas from me at this moment. Maybe others have
completely different and better ideas on how to implement this.


  Alexander