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

[Mark Gaiser <markg85@gmail.com>]

On Tue, Apr 5, 2022 at 11:27 PM Mark Gaiser <markg85@gmail.com> wrote:

>
>
> On Tue, Apr 5, 2022 at 11:01 PM Michael Niedermayer <
> michael@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?
>

Just checked patchwork:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220403223825.26764-2-markg85@gmail.com/
It also shows the indentation as I intended which should be according to
the ffmpeg coding style guidelines.

Same with:
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-April/294932.html


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