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

[Mark Gaiser <markg85@gmail.com>]

On Wed, Apr 6, 2022 at 4:18 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com>
wrote:

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

Ah right.
Well, I did have a clang-format I used very early on but never since.

That along with git log -p helped :)
V13 coming up!


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