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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 5289 bytes --]

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


[...]
> +    // 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 ?



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

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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