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

[Mark Gaiser <markg85@gmail.com>]

On Thu, Mar 31, 2022 at 11:44 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Mark Gaiser:
> > On Wed, Mar 30, 2022 at 3:57 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Mark Gaiser:
> >>> On Wed, Mar 30, 2022 at 2:21 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Mark Gaiser:
> >>>>> 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).
> >>>>>
> >>>>> Signed-off-by: Mark Gaiser <markg85@gmail.com>
> >>>>> ---
> >>>>>  configure                 |   2 +
> >>>>>  doc/protocols.texi        |  30 ++++
> >>>>>  libavformat/Makefile      |   2 +
> >>>>>  libavformat/ipfsgateway.c | 309
> ++++++++++++++++++++++++++++++++++++++
> >>>>>  libavformat/protocols.c   |   2 +
> >>>>>  5 files changed, 345 insertions(+)
> >>>>>  create mode 100644 libavformat/ipfsgateway.c
> >>>>>
> >>>>> diff --git a/configure b/configure
> >>>>> index e4d36aa639..55af90957a 100755
> >>>>> --- a/configure
> >>>>> +++ b/configure
> >>>>> @@ -3579,6 +3579,8 @@ udp_protocol_select="network"
> >>>>>  udplite_protocol_select="network"
> >>>>>  unix_protocol_deps="sys_un_h"
> >>>>>  unix_protocol_select="network"
> >>>>> +ipfs_protocol_select="https_protocol"
> >>>>> +ipns_protocol_select="https_protocol"
> >>>>>
> >>>>>  # external library protocols
> >>>>>  libamqp_protocol_deps="librabbitmq"
> >>>>> diff --git a/doc/protocols.texi b/doc/protocols.texi
> >>>>> index d207df0b52..7c9c0a4808 100644
> >>>>> --- a/doc/protocols.texi
> >>>>> +++ b/doc/protocols.texi
> >>>>> @@ -2025,5 +2025,35 @@ decoding errors.
> >>>>>
> >>>>>  @end table
> >>>>>
> >>>>> +@section ipfs
> >>>>> +
> >>>>> +InterPlanetary File System (IPFS) protocol support. One can access
> >>>> files stored
> >>>>> +on the IPFS network through so called gateways. Those are http(s)
> >>>> endpoints.
> >>>>> +This protocol wraps the IPFS native protocols (ipfs:// and ipns://)
> to
> >>>> be send
> >>>>> +to such a gateway. Users can (and should) host their own node which
> >>>> means this
> >>>>> +protocol will use your local machine gateway to access files on the
> >>>> IPFS network.
> >>>>> +
> >>>>> +If a user doesn't have a node of their own then the public gateway
> >>>> dweb.link is
> >>>>> +used by default.
> >>>>> +
> >>>>> +You can use this protocol in 2 ways. Using IPFS:
> >>>>> +@example
> >>>>> +ffplay ipfs://QmbGtJg23skhvFmu9mJiePVByhfzu5rwo74MEkVDYAmF5T
> >>>>> +@end example
> >>>>> +
> >>>>> +Or the IPNS protocol (IPNS is mutable IPFS):
> >>>>> +@example
> >>>>> +ffplay ipns://QmbGtJg23skhvFmu9mJiePVByhfzu5rwo74MEkVDYAmF5T
> >>>>> +@end example
> >>>>> +
> >>>>> +You can also change the gateway to be used:
> >>>>> +
> >>>>> +@table @option
> >>>>> +
> >>>>> +@item gateway
> >>>>> +Defines the gateway to use. When nothing is provided the protocol
> will
> >>>> first try
> >>>>> +your local gateway. If that fails dweb.link will be used.
> >>>>> +
> >>>>> +@end table
> >>>>>
> >>>>>  @c man end PROTOCOLS
> >>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
> >>>>> index d7182d6bd8..e3233fd7ac 100644
> >>>>> --- a/libavformat/Makefile
> >>>>> +++ b/libavformat/Makefile
> >>>>> @@ -660,6 +660,8 @@ OBJS-$(CONFIG_SRTP_PROTOCOL)             +=
> >>>> srtpproto.o srtp.o
> >>>>>  OBJS-$(CONFIG_SUBFILE_PROTOCOL)          += subfile.o
> >>>>>  OBJS-$(CONFIG_TEE_PROTOCOL)              += teeproto.o tee_common.o
> >>>>>  OBJS-$(CONFIG_TCP_PROTOCOL)              += tcp.o
> >>>>> +OBJS-$(CONFIG_IPFS_PROTOCOL)             += ipfsgateway.o
> >>>>> +OBJS-$(CONFIG_IPNS_PROTOCOL)             += ipfsgateway.o
> >>>>>  TLS-OBJS-$(CONFIG_GNUTLS)                += tls_gnutls.o
> >>>>>  TLS-OBJS-$(CONFIG_LIBTLS)                += tls_libtls.o
> >>>>>  TLS-OBJS-$(CONFIG_MBEDTLS)               += tls_mbedtls.o
> >>>>> diff --git a/libavformat/ipfsgateway.c b/libavformat/ipfsgateway.c
> >>>>> new file mode 100644
> >>>>> index 0000000000..1a039589c0
> >>>>> --- /dev/null
> >>>>> +++ b/libavformat/ipfsgateway.c
> >>>>> @@ -0,0 +1,309 @@
> >>>>> +/*
> >>>>> + * IPFS and IPNS protocol support through IPFS Gateway.
> >>>>> + * Copyright (c) 2022 Mark Gaiser
> >>>>> + *
> >>>>> + * This file is part of FFmpeg.
> >>>>> + *
> >>>>> + * FFmpeg is free software; you can redistribute it and/or
> >>>>> + * modify it under the terms of the GNU Lesser General Public
> >>>>> + * License as published by the Free Software Foundation; either
> >>>>> + * version 2.1 of the License, or (at your option) any later
> version.
> >>>>> + *
> >>>>> + * FFmpeg is distributed in the hope that it will be useful,
> >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>>>> + * Lesser General Public License for more details.
> >>>>> + *
> >>>>> + * You should have received a copy of the GNU Lesser General Public
> >>>>> + * License along with FFmpeg; if not, write to the Free Software
> >>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> >>>> 02110-1301 USA
> >>>>> + */
> >>>>> +
> >>>>> +#include "avformat.h"
> >>>>> +#include "libavutil/avassert.h"
> >>>>
> >>>> Unused.
> >>>>
> >>>>> +#include "libavutil/avstring.h"
> >>>>> +#include "libavutil/internal.h"
> >>>>> +#include "libavutil/opt.h"
> >>>>> +#include "libavutil/tree.h"
> >>>>
> >>>> ?
> >>>>
> >>>
> >>> This whole include part can be cleaned up much more.
> >>> Just having these:
> >>> #include "libavutil/avstring.h"
> >>> #include "libavutil/opt.h"
> >>> #include "url.h"
> >>> #include <sys/stat.h>
> >>>
> >>> is enough.
> >>>
> >>>
> >>>>> +#include <fcntl.h>
> >>>>> +#if HAVE_IO_H
> >>>>> +#include <io.h>
> >>>>> +#endif
> >>>>> +#if HAVE_UNISTD_H
> >>>>> +#include <unistd.h>
> >>>>> +#endif
> >>>>> +#include "os_support.h"
> >>>>> +#include "url.h"
> >>>>> +#include <stdlib.h>
> >>>>> +#include <sys/stat.h>
> >>>>> +
> >>>>> +typedef struct IPFSGatewayContext {
> >>>>> +    AVClass *class;
> >>>>> +    URLContext *inner;
> >>>>> +    // Is filled by the -gateway argument and not changed after.
> >>>>> +    char *gateway;
> >>>>> +    // If the above gateway is non null, it will be copied into this
> >>>> buffer.
> >>>>> +    // Else this buffer will contain the auto detected gateway.
> >>>>> +    // In either case, the gateway to use will be in this buffer.
> >>>>> +    char gateway_buffer[PATH_MAX];
> >>>>> +} IPFSGatewayContext;
> >>>>> +
> >>>>> +// A best-effort way to find the IPFS gateway.
> >>>>> +// Only the most appropiate gateway is set. It's not actually
> >> requested
> >>>>> +// (http call) to prevent a potential slowdown in startup. A
> potential
> >>>> timeout
> >>>>> +// is handled by the HTTP protocol.
> >>>>> +static int populate_ipfs_gateway(URLContext *h)
> >>>>> +{
> >>>>> +    IPFSGatewayContext *c = h->priv_data;
> >>>>> +    char ipfs_full_data_folder[PATH_MAX];
> >>>>> +    char ipfs_gateway_file[PATH_MAX];
> >>>>> +    struct stat st;
> >>>>> +    int stat_ret = 0;
> >>>>> +    int ret = AVERROR(EINVAL);
> >>>>> +    FILE *gateway_file = NULL;
> >>>>> +
> >>>>> +    // Test $IPFS_GATEWAY.
> >>>>> +    if (getenv("IPFS_GATEWAY") != NULL) {
> >>>>> +        if (snprintf(c->gateway_buffer, sizeof(c->gateway_buffer),
> >> "%s",
> >>>>> +                     getenv("IPFS_GATEWAY")) >=
> >>>> sizeof(c->gateway_buffer)) {
> >>>>> +            av_log(h, AV_LOG_WARNING, "The IPFS_GATEWAY environment
> >>>> variable exceeds the maximum length. We allow a max of %zu
> >> characters\n",
> >>>> sizeof(c->gateway_buffer));
> >>>>> +            ret = AVERROR(EINVAL);
> >>>>> +            goto err;
> >>>>> +        }
> >>>>> +
> >>>>> +        ret = 1;
> >>>>> +        goto err;
> >>>>> +    } else
> >>>>> +        av_log(h, AV_LOG_DEBUG, "$IPFS_GATEWAY is empty.\n");
> >>>>> +
> >>>>> +    // We need to know the IPFS folder to - eventually - read the
> >>>> contents of
> >>>>> +    // the "gateway" file which would tell us the gateway to use.
> >>>>> +    if (getenv("IPFS_PATH") == NULL) {
> >>>>> +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> >>>>> +
> >>>>> +        // Try via the home folder.
> >>>>> +        if (getenv("HOME") == NULL) {
> >>>>> +            av_log(h, AV_LOG_WARNING, "$HOME appears to be
> empty.\n");
> >>>>> +            ret = AVERROR(EINVAL);
> >>>>> +            goto err;
> >>>>> +        }
> >>>>> +
> >>>>> +        // Verify the composed path fits.
> >>>>> +        if (snprintf(ipfs_full_data_folder,
> >>>> sizeof(ipfs_full_data_folder),
> >>>>> +                     "%s/.ipfs/", getenv("HOME")) >=
> >>>> sizeof(ipfs_full_data_folder)) {
> >>>>> +            av_log(h, AV_LOG_WARNING, "The IPFS data path exceeds
> the
> >>>> max path length (%zu)\n", sizeof(ipfs_full_data_folder));
> >>>>> +            ret = AVERROR(EINVAL);
> >>>>> +            goto err;
> >>>>> +        }
> >>>>> +
> >>>>> +        // Stat the folder.
> >>>>> +        // It should exist in a default IPFS setup when run as local
> >>>> user.
> >>>>> +#ifndef _WIN32
> >>>>> +        stat_ret = stat(ipfs_full_data_folder, &st);
> >>>>> +#else
> >>>>> +        stat_ret = win32_stat(ipfs_full_data_folder, &st);
> >>>>> +#endif
> >>>>> +        if (stat_ret < 0) {
> >>>>> +            av_log(h, AV_LOG_INFO, "Unable to find IPFS folder. We
> >>>> tried:\n");
> >>>>> +            av_log(h, AV_LOG_INFO, "- $IPFS_PATH, which was
> >> empty.\n");
> >>>>> +            av_log(h, AV_LOG_INFO, "- $HOME/.ipfs (full uri: %s)
> which
> >>>> doesn't exist.\n", ipfs_full_data_folder);
> >>>>> +            ret = AVERROR(ENOENT);
> >>>>> +            goto err;
> >>>>> +        }
> >>>>> +    } else {
> >>>>> +        if (snprintf(ipfs_full_data_folder,
> >>>> sizeof(ipfs_full_data_folder), "%s",
> >>>>> +                 getenv("IPFS_PATH")) >=
> >> sizeof(ipfs_full_data_folder))
> >>>> {
> >>>>> +            av_log(h, AV_LOG_WARNING, "The IPFS_PATH environment
> >>>> variable exceeds the maximum length. We allow a max of %zu
> >> characters\n",
> >>>> sizeof(c->gateway_buffer));
> >>>>> +            ret = AVERROR(EINVAL);
> >>>>> +            goto err;
> >>>>> +        }
> >>>>> +
> >>>>> +    }
> >>>>> +
> >>>>> +    // Copy the fully composed gateway path into ipfs_gateway_file.
> >>>>> +    if (snprintf(ipfs_gateway_file, sizeof(ipfs_gateway_file),
> >>>> "%sgateway",
> >>>>> +                 ipfs_full_data_folder) >=
> sizeof(ipfs_gateway_file))
> >> {
> >>>>> +        av_log(h, AV_LOG_WARNING, "The IPFS gateway file path
> exceeds
> >>>> the max path length (%zu)\n", sizeof(ipfs_gateway_file));
> >>>>> +        ret = AVERROR(ENOENT);
> >>>>> +        goto err;
> >>>>> +    }
> >>>>> +
> >>>>> +    // Get the contents of the gateway file.
> >>>>> +    gateway_file = av_fopen_utf8(ipfs_gateway_file, "r");
> >>>>> +    if (!gateway_file) {
> >>>>> +        av_log(h, AV_LOG_WARNING, "The IPFS gateway file (full uri:
> >> %s)
> >>>> doesn't exist. Is the gateway enabled?\n", ipfs_gateway_file);
> >>>>> +        ret = AVERROR(ENOENT);
> >>>>> +        goto err;
> >>>>> +    }
> >>>>> +
> >>>>> +    // Read a single line (fgets stops at new line mark).
> >>>>> +    fgets(c->gateway_buffer, sizeof(c->gateway_buffer) - 1,
> >>>> gateway_file);
> >>>>> +
> >>>>> +    // Replace the last char with \0
> >>>>> +    c->gateway_buffer[sizeof(c->gateway_buffer) - 1] = 0;
>
> Btw: fgets normally zero-terminates the buffer (i.e. it reads at most
> (sizeof(c->gateway_buffer) - 1) - 1 characters from the stream and adds
> a \0. But on a read error it doesn't do this, instead the contents of
> the buffer are indeterminate and so should not be used. Adding a zero at
> the very end of gateway_buffer does not help to enforce this.


> fgets returns NULL on read error and if nothing could be read due to
> immediate EOF; so you should check for that.
> (Btw: A protocol's context is zeroed initially, so
> c->gateway_buffer[sizeof(c->gateway_buffer) - 1] is already zero,
> because no one ever wrote anything else there.)
>

Nice one! Nobody caught that before.
So checking on NULL for fgets and removing the line to add a 0 at the end
is the appropriate fix here I assume.

Changed to what I assumed. Please correct me if I'm wrong.


>
> >>>>> +
> >>>>> +    // Replace first occurence of end of line with \0
> >>>>> +    c->gateway_buffer[strcspn(c->gateway_buffer, "\r")] = 0;
> >>>>> +    c->gateway_buffer[strcspn(c->gateway_buffer, "\n")] = 0;
> >>>>
> >>>> If the buffer contains both \r and \n and the first \n precedes the
> >>>> first \r, then the above zeroes both the first \r and the first \n. If
> >>>> it is enough to only zero the first newline, then this can be
> simplified
> >>>> to "c->gateway_buffer[strcspn(c->gateway_buffer, "\r\n")] = 0;".
> >>>>
> >>>
> >>> It used to be in your suggested simplified approach.
> >>> It was then suggested to split it because of new line differences
> >>> on different platforms. This was supposed to catch "everything".
> >>>
> >>> I prefer to keep this as-is now.
> >>>
> >>
> >> Is it intended to use what is between the first \n and the first \r
> >> lateron although this data will be after the '\0' after this? That would
> >> be very weird.
> >>
> >
> > You have to elaborate on that as that sounds really wrong to me.
> >
>
> c->gateway_buffer[strcspn(c->gateway_buffer, "\r\n")] = 0;
> overwrites the first \r or \n (whichever is first) with '\0' (if any;
> otherwise it overwrites the terminating \0 with \0.
>
> c->gateway_buffer[strcspn(c->gateway_buffer, "\r")] = 0;
> c->gateway_buffer[strcspn(c->gateway_buffer, "\n")] = 0;
> also does this, but in case that the string originally contained both a
> \r and a \n with the first \n preceding the first \r it would also zero
> the first \r; this position is after the first terminating \0 (those at
> the position where the first \n was), so it appeared to me that you
> intended to use what is after the first \0. Because that is the only
> difference between the approach with two calls to strcspn and the one
> with one call to strcspn.
>
> But then I read that this has been suggested to you and found the mail
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2022-February/292590.html)
> this referred to. There seems to be a misunderstanding in the semantics
> of strcspn (most likely a confusion with strstr):
> strcspn(c->gateway_buffer, "\r\n") works with single \r and single \n,
> there need not be a single "\r\n" substring. Both methods would produce
> strings that compare equal according to strcmp().
>
>
Thank you for your elaborate explanation here!
But now I'm not sure what the resolution - if any - should be?


> > If I'm correct a file doesn't start with either \r or \n. Unless you have
> > intentional line breaks.
> > It ends with those, not starts. I'm just going to ignore \r for now for
> > brevity.
> >
> > So if you have:
> > \n<your gateway>\n
> >
> > Would be translated to:
> > \0<your gateway>\n
> >
> > Then indeed you won't get a gateway because it's intended that your first
> > line is that gateway.
> >
> > It must be in this format:
> > <your gateway>\n
> >
> > or without a newline
> > <your gateway>
> >
> > There is no blank line trimming to fix user screwups.
> > Users are not intended to touch that file anyhow. It's auto-generated by
> > IPFS and removed once it shuts down.
> >
> >
> >>>
> >>>>> +
> >>>>> +    // If strlen finds anything longer then 0 characters then we
> have
> >> a
> >>>>> +    // potential gateway url.
> >>>>> +    if (strlen(c->gateway_buffer) < 1) {
> >>>>
> >>>> if (*c->gateway_buffer == '\0')
> >>>>
> >>>
> >>> Is that a style difference or an actual behavior difference?
> >>>
> >>
> >> The former would call strlen to check whether a string is empty (unless
> >> the compiler optimizes it away). No actual behaviour difference exists.
> >> But it is nevertheless more than a style difference.
> >>
> >
> > Fixed locally.
> >
> >>
> >>>
> >>>>
> >>>>> +        av_log(h, AV_LOG_WARNING, "The IPFS gateway file (full uri:
> >> %s)
> >>>> appears to be empty. Is the gateway started?\n", ipfs_gateway_file);
> >>>>> +        ret = AVERROR(EILSEQ);
> >>>>> +        goto err;
> >>>>> +    } else {
> >>>>> +        // We're done, the c->gateway_buffer has something that
> looks
> >>>> valid.
> >>>>> +        ret = 1;
> >>>>> +        goto err;
> >>>>> +    }
> >>>>> +
> >>>>> +err:
> >>>>> +    if (gateway_file)
> >>>>> +        fclose(gateway_file);
> >>>>> +
> >>>>> +    return ret;
> >>>>> +}
> >>>>> +
> >>>>> +static int translate_ipfs_to_http(URLContext *h, const char *uri,
> >>>>> +                                  int flags, AVDictionary **options)
> >>>>> +{
> >>>>> +    const char *ipfs_cid;
> >>>>> +    char *fulluri = NULL;
> >>>>> +    int ret;
> >>>>> +    IPFSGatewayContext *c = h->priv_data;
> >>>>> +
> >>>>> +    // Test for ipfs://, ipfs:, ipns:// and ipns:. This prefix is
> >>>> stripped from
> >>>>> +    // the string leaving just the CID in ipfs_cid.
> >>>>> +    int is_ipfs = av_stristart(uri, "ipfs://", &ipfs_cid);
> >>>>> +    int is_ipns = av_stristart(uri, "ipns://", &ipfs_cid);
> >>>>> +
> >>>>> +    // We must have either ipns or ipfs.
> >>>>> +    if (!is_ipfs && !is_ipns) {
> >>>>> +        ret = AVERROR(EINVAL);
> >>>>> +        av_log(h, AV_LOG_WARNING, "Unsupported url %s\n", uri);
> >>>>> +        goto err;
> >>>>> +    }
> >>>>> +
> >>>>> +    // If the CID has a length greater then 0 then we assume we
> have a
> >>>> proper working one.
> >>>>> +    // It could still be wrong but in that case the gateway should
> >> save
> >>>> us and
> >>>>> +    // ruturn a 403 error. The http protocol handles this.
> >>>>> +    if (strlen(ipfs_cid) < 1) {
> >>>>> +        av_log(h, AV_LOG_WARNING, "A CID must be provided.\n");
> >>>>> +        ret = AVERROR(EILSEQ);
> >>>>> +        goto err;
> >>>>> +    }
> >>>>> +
> >>>>> +    // 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));
> >>>>
> >>>> We typically use SIZE_SPECIFIER instead of z for compatibility with
> >>>> ancient versions of MSVC.
> >>>> (I don't know whether there is any supported version of MSVC that
> >>>> doesn't support z; I don't use MSVC myself.)
> >>>>
> >>>
> >>> Here too I was explicitly suggested to use %zu (when I was using - I
> >> think
> >>> - %lu before).
> >>> So I assume that the ancient MSVC version you're referring to is
> probably
> >>> not supported anymore from an ffmpeg compiler requirement point of
> view?
> >>>
> >>> Should i now change all "%zu" to "%"SIZE_SPECIFIER (this does not make
> it
> >>> neater nor shorter with the 80 char line limit).
> >>> Is this change required?
> >>
> >> As said: It is for compatibility with ancient versions of MSVC. But I
> >> don't know whether any of the actually supported versions of MSVC still
> >> need it.
> >>
> >>>
> >>> If it is, what _exactly_ do i need to change it in? I see a
> >>> couple different SIZE_SPECIFIER prefixes. I have no clue what to use
> >> here.
> >>
> >> There is only one SIZE_SPECIFIER, namely SIZE_SPECIFIER; the
> >> PTRDIFF_SPECIFIER (or whatever you see) is obviously not the thing to
> >> use for size_t.
> >>
> >> I see some:
> > %"SIZE_SPECIFIER"
> >
> > But also like this:
> > %8"SIZE_SPECIFIER
> > %5"SIZE_SPECIFIER
> >
> > I'm assuming they are for clipping?
>
> printf specifiers consist of flags, minimum field width, precision
> (indicated by '.'), length modifier and conversion specifier (in that
> order). Like the macros in inttypes.h SIZE_SPECIFIER expands to a
> character string literal that includes a length modifier and a
> conversion specifier, so that one can use this together with the former
> three possibilities.
> Of course, it would make no sense to use any of these three here, so it
> would just be %"SIZE_SPECIFIER".
>
> >
> > I really don't like having to change it to this as it:
> > - Uglifies the code (and i doubt if it's even needed)
>
> I agree to both points. Which is why I am not forcing you to do this. It
> seems that there is already at least one commonly compiled instance
> where it is not used at all (libavformat/argo_cvg.c).
>

So i leave it as "%zu". Check.
I did also grep on the entire ffmpeg source for this. Both seems to be used
in various places though granted, SIZE_SPECIFIER is used more.

>
> > - Directly contradicts earlier feedback
>
> I don't agree with that given that SIZE_SPECIFIER is basically just a
> wrapper around zu.
>

My mistake, it doesn't contradict it. Sorry for that.

>
> > - And due to the above gives me a feeling of wasting time
> > - Fine to change it but I hope there isn't another person popping up
> with a
> > question to change it yet again....
> >
> >
> >>>
> >>>
> >>>>> +            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");
> >>>>> +            av_log(h, AV_LOG_INFO, "There are multiple options to
> >>>> define this gateway.\n");
> >>>>> +            av_log(h, AV_LOG_INFO, "1. Call ffmpeg with a gateway
> >>>> param, without a trailing slash: -gateway <url>.\n");
> >>>>> +            av_log(h, AV_LOG_INFO, "2. Define an $IPFS_GATEWAY
> >>>> environment variable with the full HTTP URL to the gateway without
> >> trailing
> >>>> forward slash.\n");
> >>>>> +            av_log(h, AV_LOG_INFO, "3. Define an $IPFS_PATH
> >> environment
> >>>> variable and point it to the IPFS data path - this is typically
> >> ~/.ipfs\n");
> >>>>
> >>>> All those AV_LOG_INFO can be combined which has the advantage that the
> >>>> logs can't be teared apart (which they can now if something else logs
> at
> >>>> the same time); furthermore, this would also decrease codesize.
> >>>>
> >>>
> >>> Do you have an example of where that's happening?
> >>>
> >>
> >> It can happen any time you have multiple threads using av_log at the
> >> same time. Given that your statements are supposed to be full lines, it
> >> is not that bad if it happens here, but it is nevertheless suboptimal.
> >>
> >
> > I meant a multi-line av_log example ;) But yeah, I get how threading
> could
> > screw this up.
> >
>
> I meant something like:
> 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");
>
> (These lines are actually way too long.)
>

Fixed as multiline.
I do like to keep the content of the lines though.
I also checked this with IPFS folks to be as correct and clear as we could
make it.

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