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

["\"zhilizhao(赵志立)\"" <quinkblack@foxmail.com>]

> On Apr 6, 2022, at 5:34 AM, Mark Gaiser <markg85@gmail.com> wrote:
> 
> 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.

Indent size is 4. I use git log -p to do one more check before sending
patches.

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

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