[FFmpeg-devel] [PATCH v12 1/1] avformat: Add IPFS protocol support.
"zhilizhao(赵志立)"
quinkblack at foxmail.com
Wed Apr 6 05:17:59 EEST 2022
> On Apr 6, 2022, at 5:34 AM, Mark Gaiser <markg85 at gmail.com> wrote:
>
> On Tue, Apr 5, 2022 at 11:27 PM Mark Gaiser <markg85 at gmail.com> wrote:
>
>>
>>
>> On Tue, Apr 5, 2022 at 11:01 PM Michael Niedermayer <
>> michael at 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 at ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>>
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list