On Wed, Apr 6, 2022 at 4:18 AM "zhilizhao(赵志立)" <quinkbl...@foxmail.com>
wrote:
>
>
> > On Apr 6, 2022, at 5:34 AM, Mark Gaiser <mark...@gmail.com> wrote:
> >
> > On Tue, Apr 5, 2022 at 11:27 PM Mark Gaiser <mark...@gmail.com> wrote:
> >
> >>
> >>
> >> On Tue, Apr 5, 2022 at 11:01 PM Michael Niedermayer <
> >> mich...@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-mark...@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-requ...@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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
