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

[Mark Gaiser <markg85@gmail.com>]

On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen@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.

>
> > +    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?
In a C++ world I'd use a scoped variable ;) But I kinda miss how to do that
properly here.


> > +
> > +        // Stat the folder. It should exist in a default IPFS setup
> > when run as local user.
> > +#ifndef _WIN32
> > +        stat_ret = stat(ipfs_full_data_folder, &st);
> > +#else
> > +        stat_ret = win32_stat(ipfs_full_data_folder, &st);
> > +#endif
>
> Why bother with stat() when you can just check whether fopen()
> succeeded?
>

Ohh! Nice one!
It doesn't make the code shorter or easier though. But does get rid of
platform dependent stuff so a win imho anyhow.

>
> > +// For now just makes sure that the gateway ends in url we expect.
> > Like http://localhost:8080/.
> > +// Explicitly with the traling slash.
> > +static void ff_sanitize_ipfs_gateway(URLContext *h)
> > +{
> > +    Context *c = h->priv_data;
> > +    const char last_gateway_char = c->gateway[strlen(c->gateway) -
> > 1];
>
> Can strlen(c->gateway) be zero here?
>

Hmm, potentially yes. I'll add a check for it.

>
> > +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://?


>
> Also what happens if c->gateway is "ipfs://[...]"? Infinite recursion?
>

Nice one, I need to test that!

>
> /Tomas
>
>
> _______________________________________________
> 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".