[FFmpeg-devel] [PATCH v2 1/1] avformat: Add IPFS protocol support.

[Mark Gaiser]

On Fri, Feb 4, 2022 at 11:29 AM Tomas Härdin <tjoppen at acc.umu.se> wrote:

> ons 2022-02-02 klockan 14:56 +0100 skrev Mark Gaiser:
> > On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen at acc.umu.se>
> > wrote:
> >
> > > tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:
> > >
> > > >
> > > > +typedef struct Context {
> > > > +    AVClass *class;
> > > > +    URLContext *inner;
> > > > +    char *gateway;
> > >
> > > Is there not a maximum length that an HTTP URL can be? At least
> > > without
> > > query parameters. That way you avoid dynamic allocations. You'd
> > > have to
> > > separate the AVOption from such a buffer in that case, but I think
> > > you
> > > have to anyway.
> > >
> >
> > Could you provide more information on that? Or an example of what you
> > mean
> > exactly?
> > As far as i know there is no hard limit though it's very much advised
> > to
> > not go above 2048 characters.
>
> You could at least separate the variable that is set by the AVOption
> stuff from the buffer that's produced by the URL parsing stuff.
>
> Since you call ffurl_open_whitelist() immediately you could just use
> alloca() to allocate memory on the stack. That way you don't have to
> worry about deallocation at all *and* you can have it be arbitrarily
> large.
>
> If URLs have a spec'd maximum length then a buffer of constant size on
> the stack would work.
>
> >
> > >
> > > > +    if (!ipfs_full_data_folder) {
> > > > +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> > > > +
> > > > +        // Try via the home folder.
> > > > +        home_folder = getenv("HOME");
> > > > +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
> > > > home_folder);
> > >
> > > Memory leak. This applies to most if not all av_asprintf() calls.
> > >
> >
> > Is there an advised way to neatly clean that up?
> > Sure, I can add a bunch of av_free calls to clean it up. But there
> > are
> > places where it's not as straightforward like where the av_asprintf
> > was
> > done in an if statement. How do I maintain the knowledge that
> > av_asprintf
> > was used to call av_free later?
>
> goto as the others pointed out
>
>
> >
> > >
> > > > +static int translate_ipfs_to_http(URLContext *h, const char
> > > > *uri,
> > > > int flags, AVDictionary **options)
> > > > +{
> > > > +    const char *ipfs_cid;
> > > > +    const char *protocol_path_suffix = "ipfs/";
> > > > +    char *fulluri;
> > > > +    int ret;
> > > > +    Context *c = h->priv_data;
> > > > +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> > > > av_strstart(uri, "ipfs:", &ipfs_cid));
> > > > +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> > > > av_strstart(uri, "ipns:", &ipfs_cid));
> > >
> > > https://docs.ipfs.io/concepts/ipfs-gateway/ claims ipfs:// is the
> > > canonical form. No mentioned is made of any ipfs:{CID} form.
> > > Incorrect
> > > URLs should be rejected, not silently patched.
> > >
> >
> > I'd like to make a decision here. This current logic (ipfs:// and
> > ipfs:,
> > same for ipns) is inspired by other protocols that ffmpeg supported.
> > I
> > simply copied how they work to be consistent.
> > Do i:
> > 1. keep it as is and be consistent with the rest?
> > 2. only allow ipfs:// and ipns://?
> >
>
> Which protocols? This laxness in lavf is worrisome. It breeds laxness
> in other projects.
>

Hmm, I might have been looking at the wrong example and calling that
"others do it too".
Crypto does it with "crypto:" and "crypto+"

>From a quick glance that's about it.

I'll just remove "ipfs:". I only wanted to stay consistent with what I knew
(from crypto) but that is apparently inconsistent.



>
> /Tomas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>