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

[Mark Gaiser]

On Tue, Apr 5, 2022 at 11:01 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Mon, Apr 04, 2022 at 12:38:25AM +0200, Mark Gaiser wrote:
> > This patch adds support for:
> > - ffplay ipfs://<cid>
> > - ffplay ipns://<cid>
> >
> > IPFS data can be played from so called "ipfs gateways".
> > A gateway is essentially a webserver that gives access to the
> > distributed IPFS network.
> >
> > This protocol support (ipfs and ipns) therefore translates
> > ipfs:// and ipns:// to a http:// url. This resulting url is
> > then handled by the http protocol. It could also be https
> > depending on the gateway provided.
> >
> > To use this protocol, a gateway must be provided.
> > If you do nothing it will try to find it in your
> > $HOME/.ipfs/gateway file. The ways to set it manually are:
> > 1. Define a -gateway <url> to the gateway.
> > 2. Define $IPFS_GATEWAY with the full http link to the gateway.
> > 3. Define $IPFS_PATH and point it to the IPFS data path.
> > 4. Have IPFS running in your local user folder (under $HOME/.ipfs).
> >
> [...]
>
> > +        goto err;
> > +    }
> > +
> > +    // Read a single line (fgets stops at new line mark).
> > +    if (!fgets(c->gateway_buffer, sizeof(c->gateway_buffer) - 1,
> gateway_file)) {
> > +      av_log(h, AV_LOG_WARNING, "Unable to read from file (full uri:
> %s).\n",
> > +             ipfs_gateway_file);
> > +      ret = AVERROR(ENOENT);
> > +      goto err;
> > +    }
>
> The indention is not consistent
>

What's the intended indentation here?
In my editor (QtCreator, it's set to 2 spaces for tabs) the
"ipfs_gateway_file" is aligned directly underneath the first argument of
av_log.
That is as it should be, right?

For this and your other comments, I see no issue on my side. Also no
trailing whitespace.

Here's an image of what i see with spaces visualizes:
https://i.imgur.com/37k68RH.png
Is there something wrong on my end?


>
>
> [...]
> > +    // Populate c->gateway_buffer with whatever is in c->gateway
> > +    if (c->gateway != NULL) {
> > +        if (snprintf(c->gateway_buffer, sizeof(c->gateway_buffer), "%s",
> > +                     c->gateway) >= sizeof(c->gateway_buffer)) {
> > +            av_log(h, AV_LOG_WARNING, "The -gateway parameter is too
> long. "
> > +                                      "We allow a max of %zu
> characters\n",
> > +                   sizeof(c->gateway_buffer));
> > +            ret = AVERROR(EINVAL);
> > +            goto err;
> > +        }
> > +    } else {
> > +        // Populate the IPFS gateway if we have any.
> > +        // If not, inform the user how to properly set one.
> > +        ret = populate_ipfs_gateway(h);
> > +
> > +        if (ret < 1) {
> > +            // We fallback on dweb.link (managed by Protocol Labs).
> > +            snprintf(c->gateway_buffer, sizeof(c->gateway_buffer), "
> https://dweb.link");
> > +
> > +            av_log(h, AV_LOG_WARNING, "IPFS does not appear to be
> running. "
> > +                                      "You’re now using the public
> gateway at dweb.link.\n");
> > +            av_log(h, AV_LOG_INFO, "Installing IPFS locally is
> recommended to "
> > +                                   "improve performance and
> reliability, "
> > +                                   "and not share all your activity
> with a single IPFS gateway.\n"
> > +                   "There are multiple options to define this
> gateway.\n"
> > +                   "1. Call ffmpeg with a gateway param, "
> > +                                   "without a trailing slash: -gateway
> <url>.\n"
> > +                   "2. Define an $IPFS_GATEWAY environment variable
> with the "
> > +                                   "full HTTP URL to the gateway "
> > +                                   "without trailing forward slash.\n"
> > +                   "3. Define an $IPFS_PATH environment variable "
> > +                                   "and point it to the IPFS data path "
> > +                                   "- this is typically ~/.ipfs\n");
> > +        }
> > +    }
>
> This will print the warning every time a ipfs url is opened. Not just once
> is that intended ?
>

Yes.

Or rather, I don't see how to make it persistent in a nice intuitive way.
By nice intuitive I mean showing it for, lets say, 10 times you use ffmpeg
to be "sure" you've seen it before it can stop annoying the user about it.

Adding complexity for that doesn't seem to be worth it to me.


>
>
>
> > +
> > +    // Test if the gateway starts with either http:// or https://
> > +    if (av_stristart(c->gateway_buffer, "http://", NULL) == 0
> > +        && av_stristart(c->gateway_buffer, "https://", NULL) == 0) {
> > +        av_log(h, AV_LOG_WARNING, "The gateway URL didn't start with
> http:// or "
> > +                                  "https:// and is therefore
> invalid.\n");
> > +        ret = AVERROR(EILSEQ);
> > +        goto err;
> > +    }
> > +
> > +    // Concatenate the url.
> > +    // This ends up with something like:
> http://localhost:8080/ipfs/Qm.....
> > +    // The format of "%s%s%s%s" is the following:
> > +    // 1st %s = The gateway.
> > +    // 2nd %s = If the gateway didn't end in a slash, add a "/".
> Otherwise it's an empty string
> > +    // 3rd %s = Either ipns/ or ipfs/.
> > +    // 4th %s = The IPFS CID (Qm..., bafy..., ...).
> > +    fulluri = av_asprintf("%s%s%s%s",
> > +                          c->gateway_buffer,
> > +                          (c->gateway_buffer[strlen(c->gateway_buffer)
> - 1] == '/') ? "" : "/",
> > +                          (is_ipns) ? "ipns/" : "ipfs/",
> > +                          ipfs_cid);
> > +
> > +    if (!fulluri) {
> > +      av_log(h, AV_LOG_ERROR, "Failed to compose the URL\n");
> > +      ret = AVERROR(ENOMEM);
> > +      goto err;
> > +    }
>
> another indention case
>
> Theres also some trailing whitespace in the patch left
>
>
> No more comments from me. LGTM after these to me i think
>

Awesome!
Still a V13 might be needed...
Though that might depend on your answer to my comment above with the image
link.


>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Many that live deserve death. And some that die deserve life. Can you give
> it to them? Then do not be too eager to deal out death in judgement. For
> even the very wise cannot see all ends. -- Gandalf
> _______________________________________________
> 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".
>