[FFmpeg-devel,v2,1/1] avformat: Add IPFS protocol support.

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 | 267 ++++++++++++++++++++++++++++++++++++++
 libavformat/protocols.c   |   2 +
 5 files changed, 303 insertions(+)
 create mode 100644 libavformat/ipfsgateway.c

On 01.02.2022 22:58, Mark Gaiser wrote:
> +static int translate_ipfs_to_http(URLContext *h, const char *uri, int flags, AVDictionary **options)
> +{
> +    const char *ipfs_cid;
> +    const char *protocol_path_suffix = "ipfs/";
> +    char *fulluri;
> +    int ret;
> +    Context *c = h->priv_data;
> +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) || av_strstart(uri, "ipfs:", &ipfs_cid));
> +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) || av_strstart(uri, "ipns:", &ipfs_cid));

What's the point of this logic?
The first half of each check seems pointless, since the second half is 
true for everything the first one would cover.

On Wed, Feb 2, 2022 at 1:27 AM Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 01.02.2022 22:58, Mark Gaiser wrote:
> > +static int translate_ipfs_to_http(URLContext *h, const char *uri, int
> flags, AVDictionary **options)
> > +{
> > +    const char *ipfs_cid;
> > +    const char *protocol_path_suffix = "ipfs/";
> > +    char *fulluri;
> > +    int ret;
> > +    Context *c = h->priv_data;
> > +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> av_strstart(uri, "ipfs:", &ipfs_cid));
> > +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> av_strstart(uri, "ipns:", &ipfs_cid));
>
> What's the point of this logic?
> The first half of each check seems pointless, since the second half is
> true for everything the first one would cover.
>

Hi Time,

The point it to allow
ipfs://<cid> and ipfs:<cid>

So for that i want to test for all possible true situations (ipfs://,
ipfs:, ipns:// and ipns:).

This is akin to other protocols who seem to do the same check. Look at
crypto.c for example.
Another point is further down where the url is composed.
If it's ipfs it becomes a url like <gateway>/ipfs
And for ipns: <gateway>/ipns


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

On Wed, Feb 2, 2022 at 1:33 AM Mark Gaiser <markg85@gmail.com> wrote:

> On Wed, Feb 2, 2022 at 1:27 AM Timo Rothenpieler <timo@rothenpieler.org>
> wrote:
>
>> On 01.02.2022 22:58, Mark Gaiser wrote:
>> > +static int translate_ipfs_to_http(URLContext *h, const char *uri, int
>> flags, AVDictionary **options)
>> > +{
>> > +    const char *ipfs_cid;
>> > +    const char *protocol_path_suffix = "ipfs/";
>> > +    char *fulluri;
>> > +    int ret;
>> > +    Context *c = h->priv_data;
>> > +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
>> av_strstart(uri, "ipfs:", &ipfs_cid));
>> > +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
>> av_strstart(uri, "ipns:", &ipfs_cid));
>>
>> What's the point of this logic?
>> The first half of each check seems pointless, since the second half is
>> true for everything the first one would cover.
>>
>
> Hi Time,
>
oops. Timo obviously. Sorry for the typo in your name.

>
> The point it to allow
> ipfs://<cid> and ipfs:<cid>
>
> So for that i want to test for all possible true situations (ipfs://,
> ipfs:, ipns:// and ipns:).
>
> This is akin to other protocols who seem to do the same check. Look at
> crypto.c for example.
> Another point is further down where the url is composed.
> If it's ipfs it becomes a url like <gateway>/ipfs
> And for ipns: <gateway>/ipns
>
>
>>
>> _______________________________________________
>> 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".
>>
>

On 02.02.2022 01:33, Mark Gaiser wrote:
> On Wed, Feb 2, 2022 at 1:27 AM Timo Rothenpieler <timo@rothenpieler.org>
> wrote:
> 
>> On 01.02.2022 22:58, Mark Gaiser wrote:
>>> +static int translate_ipfs_to_http(URLContext *h, const char *uri, int
>> flags, AVDictionary **options)
>>> +{
>>> +    const char *ipfs_cid;
>>> +    const char *protocol_path_suffix = "ipfs/";
>>> +    char *fulluri;
>>> +    int ret;
>>> +    Context *c = h->priv_data;
>>> +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
>> av_strstart(uri, "ipfs:", &ipfs_cid));
>>> +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
>> av_strstart(uri, "ipns:", &ipfs_cid));
>>
>> What's the point of this logic?
>> The first half of each check seems pointless, since the second half is
>> true for everything the first one would cover.
>>
> 
> Hi Time,
> 
> The point it to allow
> ipfs://<cid> and ipfs:<cid>
> 
> So for that i want to test for all possible true situations (ipfs://,
> ipfs:, ipns:// and ipns:).

If the url starts with "ipns://", it obviously also starts with "ipns:", 
so checking for the longer of the two is pointless.
Same for "ipfs:".

> This is akin to other protocols who seem to do the same check. Look at
> crypto.c for example.
> Another point is further down where the url is composed.
> If it's ipfs it becomes a url like <gateway>/ipfs
> And for ipns: <gateway>/ipns

Timo Rothenpieler:
> On 02.02.2022 01:33, Mark Gaiser wrote:
>> On Wed, Feb 2, 2022 at 1:27 AM Timo Rothenpieler <timo@rothenpieler.org>
>> wrote:
>>
>>> On 01.02.2022 22:58, Mark Gaiser wrote:
>>>> +static int translate_ipfs_to_http(URLContext *h, const char *uri, int
>>> flags, AVDictionary **options)
>>>> +{
>>>> +    const char *ipfs_cid;
>>>> +    const char *protocol_path_suffix = "ipfs/";
>>>> +    char *fulluri;
>>>> +    int ret;
>>>> +    Context *c = h->priv_data;
>>>> +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
>>> av_strstart(uri, "ipfs:", &ipfs_cid));
>>>> +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
>>> av_strstart(uri, "ipns:", &ipfs_cid));
>>>
>>> What's the point of this logic?
>>> The first half of each check seems pointless, since the second half is
>>> true for everything the first one would cover.
>>>
>>
>> Hi Time,
>>
>> The point it to allow
>> ipfs://<cid> and ipfs:<cid>
>>
>> So for that i want to test for all possible true situations (ipfs://,
>> ipfs:, ipns:// and ipns:).
> 
> If the url starts with "ipns://", it obviously also starts with "ipns:",
> so checking for the longer of the two is pointless.
> Same for "ipfs:".

You forgot that av_strstart() also sets ipfs_cid which is used later;
the above code exists to strip the leading protocol part away.

> 
>> This is akin to other protocols who seem to do the same check. Look at
>> crypto.c for example.
>> Another point is further down where the url is composed.
>> If it's ipfs it becomes a url like <gateway>/ipfs
>> And for ipns: <gateway>/ipns
>

On 02.02.2022 01:44, Andreas Rheinhardt wrote:
> Timo Rothenpieler:
>> On 02.02.2022 01:33, Mark Gaiser wrote:
>>> On Wed, Feb 2, 2022 at 1:27 AM Timo Rothenpieler <timo@rothenpieler.org>
>>> wrote:
>>>
>>>> On 01.02.2022 22:58, Mark Gaiser wrote:
>>>>> +static int translate_ipfs_to_http(URLContext *h, const char *uri, int
>>>> flags, AVDictionary **options)
>>>>> +{
>>>>> +    const char *ipfs_cid;
>>>>> +    const char *protocol_path_suffix = "ipfs/";
>>>>> +    char *fulluri;
>>>>> +    int ret;
>>>>> +    Context *c = h->priv_data;
>>>>> +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
>>>> av_strstart(uri, "ipfs:", &ipfs_cid));
>>>>> +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
>>>> av_strstart(uri, "ipns:", &ipfs_cid));
>>>>
>>>> What's the point of this logic?
>>>> The first half of each check seems pointless, since the second half is
>>>> true for everything the first one would cover.
>>>>
>>>
>>> Hi Time,
>>>
>>> The point it to allow
>>> ipfs://<cid> and ipfs:<cid>
>>>
>>> So for that i want to test for all possible true situations (ipfs://,
>>> ipfs:, ipns:// and ipns:).
>>
>> If the url starts with "ipns://", it obviously also starts with "ipns:",
>> so checking for the longer of the two is pointless.
>> Same for "ipfs:".
> 
> You forgot that av_strstart() also sets ipfs_cid which is used later;
> the above code exists to strip the leading protocol part away.

Ah, yeah. That's a very confusing side effect there.
I guess it's fine, but I can't say I like using it like that.
Since there's a large risk someone else might in the future think the 
same and break it.

On Wed, Feb 2, 2022 at 1:49 AM Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 02.02.2022 01:44, Andreas Rheinhardt wrote:
> > Timo Rothenpieler:
> >> On 02.02.2022 01:33, Mark Gaiser wrote:
> >>> On Wed, Feb 2, 2022 at 1:27 AM Timo Rothenpieler <
> timo@rothenpieler.org>
> >>> wrote:
> >>>
> >>>> On 01.02.2022 22:58, Mark Gaiser wrote:
> >>>>> +static int translate_ipfs_to_http(URLContext *h, const char *uri,
> int
> >>>> flags, AVDictionary **options)
> >>>>> +{
> >>>>> +    const char *ipfs_cid;
> >>>>> +    const char *protocol_path_suffix = "ipfs/";
> >>>>> +    char *fulluri;
> >>>>> +    int ret;
> >>>>> +    Context *c = h->priv_data;
> >>>>> +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> >>>> av_strstart(uri, "ipfs:", &ipfs_cid));
> >>>>> +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> >>>> av_strstart(uri, "ipns:", &ipfs_cid));
> >>>>
> >>>> What's the point of this logic?
> >>>> The first half of each check seems pointless, since the second half is
> >>>> true for everything the first one would cover.
> >>>>
> >>>
> >>> Hi Time,
> >>>
> >>> The point it to allow
> >>> ipfs://<cid> and ipfs:<cid>
> >>>
> >>> So for that i want to test for all possible true situations (ipfs://,
> >>> ipfs:, ipns:// and ipns:).
> >>
> >> If the url starts with "ipns://", it obviously also starts with "ipns:",
> >> so checking for the longer of the two is pointless.
> >> Same for "ipfs:".
> >
> > You forgot that av_strstart() also sets ipfs_cid which is used later;
> > the above code exists to strip the leading protocol part away.
>
> Ah, yeah. That's a very confusing side effect there.
> I guess it's fine, but I can't say I like using it like that.
> Since there's a large risk someone else might in the future think the
> same and break it.
>

I'm fine changing it to someone that would feel more intuitive.
Do you have a suggestion to make it better?


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

On 02.02.2022 01:50, Mark Gaiser wrote:
>> Ah, yeah. That's a very confusing side effect there.
>> I guess it's fine, but I can't say I like using it like that.
>> Since there's a large risk someone else might in the future think the
>> same and break it.
>>
> 
> I'm fine changing it to someone that would feel more intuitive.
> Do you have a suggestion to make it better?

Could just put a comment above explaining the reliance on the stripping 
the full prefix.

On Wed, Feb 2, 2022 at 1:54 AM Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 02.02.2022 01:50, Mark Gaiser wrote:
> >> Ah, yeah. That's a very confusing side effect there.
> >> I guess it's fine, but I can't say I like using it like that.
> >> Since there's a large risk someone else might in the future think the
> >> same and break it.
> >>
> >
> > I'm fine changing it to someone that would feel more intuitive.
> > Do you have a suggestion to make it better?
>
> Could just put a comment above explaining the reliance on the stripping
> the full prefix.
>

Will do. Thanx!
I'll wait for more feedback (if any) for a day or two before sending an
updated patch.

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

1 Feb 2022, 22:58 by markg85@gmail.com:

> 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 | 267 ++++++++++++++++++++++++++++++++++++++
>  libavformat/protocols.c   |   2 +
>  5 files changed, 303 insertions(+)
>  create mode 100644 libavformat/ipfsgateway.c
>

Fix all the coding style issues first...

On Wed, Feb 2, 2022 at 3:29 AM Lynne <dev@lynne.ee> wrote:

> 1 Feb 2022, 22:58 by markg85@gmail.com:
>
> > 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 | 267 ++++++++++++++++++++++++++++++++++++++
> >  libavformat/protocols.c   |   2 +
> >  5 files changed, 303 insertions(+)
> >  create mode 100644 libavformat/ipfsgateway.c
> >
>
> Fix all the coding style issues first...
>
Any hints on those? As I thought it was matching the other files (looking
at crypto.c) nicely.

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

2 Feb 2022, 03:51 by markg85@gmail.com:

> On Wed, Feb 2, 2022 at 3:29 AM Lynne <dev@lynne.ee> wrote:
>
>> 1 Feb 2022, 22:58 by markg85@gmail.com:
>>
>> > 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 | 267 ++++++++++++++++++++++++++++++++++++++
>> >  libavformat/protocols.c   |   2 +
>> >  5 files changed, 303 insertions(+)
>> >  create mode 100644 libavformat/ipfsgateway.c
>> >
>>
>> Fix all the coding style issues first...
>>
> Any hints on those? As I thought it was matching the other files (looking
> at crypto.c) nicely.
>

https://ffmpeg.org/developer.html#Coding-Rules-1
And we also don't wrap single-line statements in blocks.

tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:

> 
> +typedef struct Context {
> +    AVClass *class;
> +    URLContext *inner;
> +    char *gateway;

Is there not a maximum length that an HTTP URL can be? At least without
query parameters. That way you avoid dynamic allocations. You'd have to
separate the AVOption from such a buffer in that case, but I think you
have to anyway.

> +    if (!ipfs_full_data_folder) {
> +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> +
> +        // Try via the home folder.
> +        home_folder = getenv("HOME");
> +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
> home_folder);

Memory leak. This applies to most if not all av_asprintf() calls.

> +
> +        // 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

Why bother with stat() when you can just check whether fopen()
succeeded?

> +// For now just makes sure that the gateway ends in url we expect.
> Like http://localhost:8080/.
> +// Explicitly with the traling slash.
> +static void ff_sanitize_ipfs_gateway(URLContext *h)
> +{
> +    Context *c = h->priv_data;
> +    const char last_gateway_char = c->gateway[strlen(c->gateway) -
> 1];

Can strlen(c->gateway) be zero here?

> +static int translate_ipfs_to_http(URLContext *h, const char *uri,
> int flags, AVDictionary **options)
> +{
> +    const char *ipfs_cid;
> +    const char *protocol_path_suffix = "ipfs/";
> +    char *fulluri;
> +    int ret;
> +    Context *c = h->priv_data;
> +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> av_strstart(uri, "ipfs:", &ipfs_cid));
> +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> av_strstart(uri, "ipns:", &ipfs_cid));

https://docs.ipfs.io/concepts/ipfs-gateway/ claims ipfs:// is the
canonical form. No mentioned is made of any ipfs:{CID} form. Incorrect
URLs should be rejected, not silently patched.

Also what happens if c->gateway is "ipfs://[...]"? Infinite recursion?

/Tomas

On Tue, Feb 01, 2022 at 10:58:30PM +0100, 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).
> 
> Signed-off-by: Mark Gaiser <markg85@gmail.com>
> ---
>  configure                 |   2 +
>  doc/protocols.texi        |  30 +++++
>  libavformat/Makefile      |   2 +
>  libavformat/ipfsgateway.c | 267 ++++++++++++++++++++++++++++++++++++++
>  libavformat/protocols.c   |   2 +
>  5 files changed, 303 insertions(+)
>  create mode 100644 libavformat/ipfsgateway.c
> 
> diff --git a/configure b/configure
> index 5b19a35f59..6ff09e7974 100755
> --- a/configure
> +++ b/configure
> @@ -3585,6 +3585,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 3dc6a479cc..4edce8420f 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -656,6 +656,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..2b52fc9392
> --- /dev/null
> +++ b/libavformat/ipfsgateway.c
> @@ -0,0 +1,267 @@
> +/*
> + * 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 "libavutil/avassert.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/internal.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/tree.h"
> +#include "avformat.h"
> +#include <fcntl.h>
> +#if HAVE_IO_H
> +#include <io.h>
> +#endif
> +#if HAVE_UNISTD_H
> +#include <unistd.h>
> +#endif
> +#include <sys/stat.h>
> +#include <stdlib.h>
> +#include "os_support.h"
> +#include "url.h"
> +

> +typedef struct Context {

i also agree that the name is a bit too unspecific


> +    AVClass *class;
> +    URLContext *inner;
> +    char *gateway;
> +} Context;
> +
> +// 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.
> +//
> +// When done and the return value is 1, a potential IPFS Gateway is set in the gateway
> +// variable in the inner URLContext.

> +static int ff_populate_ipfs_gateway(URLContext *h)

ff_ prefix seems not needed


[...]

> +    FILE *gateway_file;
> +    char gateway_file_data[1000];

[...]

> +    if (!gateway_file) {
> +        av_log(h, AV_LOG_ERROR, "Unable to open the IPFS gateway file (full uri: %s).\n", ipfs_gateway_file);

> +        return -1;

more specific error codes would be better and more consistent to existing code


> +    }
> +

> +    fscanf(gateway_file, "%[^\n]", gateway_file_data);

how large is the array ?
how large is the data written into the array ?
what is the relation between the 2 ?

[...]
> +    int ret;
> +
> +    ret = ffurl_read(c->inner, buf, size);
> +
> +    return ret;

if thats all that is needed then 

return ffurl_read(c->inner, buf, size);

is simpler


thx

[...]

On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen@acc.umu.se> wrote:

> tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:
>
> >
> > +typedef struct Context {
> > +    AVClass *class;
> > +    URLContext *inner;
> > +    char *gateway;
>
> Is there not a maximum length that an HTTP URL can be? At least without
> query parameters. That way you avoid dynamic allocations. You'd have to
> separate the AVOption from such a buffer in that case, but I think you
> have to anyway.
>

Could you provide more information on that? Or an example of what you mean
exactly?
As far as i know there is no hard limit though it's very much advised to
not go above 2048 characters.

>
> > +    if (!ipfs_full_data_folder) {
> > +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> > +
> > +        // Try via the home folder.
> > +        home_folder = getenv("HOME");
> > +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
> > home_folder);
>
> Memory leak. This applies to most if not all av_asprintf() calls.
>

Is there an advised way to neatly clean that up?
Sure, I can add a bunch of av_free calls to clean it up. But there are
places where it's not as straightforward like where the av_asprintf was
done in an if statement. How do I maintain the knowledge that av_asprintf
was used to call av_free later?
In a C++ world I'd use a scoped variable ;) But I kinda miss how to do that
properly here.


> > +
> > +        // 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
>
> Why bother with stat() when you can just check whether fopen()
> succeeded?
>

Ohh! Nice one!
It doesn't make the code shorter or easier though. But does get rid of
platform dependent stuff so a win imho anyhow.

>
> > +// For now just makes sure that the gateway ends in url we expect.
> > Like http://localhost:8080/.
> > +// Explicitly with the traling slash.
> > +static void ff_sanitize_ipfs_gateway(URLContext *h)
> > +{
> > +    Context *c = h->priv_data;
> > +    const char last_gateway_char = c->gateway[strlen(c->gateway) -
> > 1];
>
> Can strlen(c->gateway) be zero here?
>

Hmm, potentially yes. I'll add a check for it.

>
> > +static int translate_ipfs_to_http(URLContext *h, const char *uri,
> > int flags, AVDictionary **options)
> > +{
> > +    const char *ipfs_cid;
> > +    const char *protocol_path_suffix = "ipfs/";
> > +    char *fulluri;
> > +    int ret;
> > +    Context *c = h->priv_data;
> > +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> > av_strstart(uri, "ipfs:", &ipfs_cid));
> > +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> > av_strstart(uri, "ipns:", &ipfs_cid));
>
> https://docs.ipfs.io/concepts/ipfs-gateway/ claims ipfs:// is the
> canonical form. No mentioned is made of any ipfs:{CID} form. Incorrect
> URLs should be rejected, not silently patched.
>

I'd like to make a decision here. This current logic (ipfs:// and ipfs:,
same for ipns) is inspired by other protocols that ffmpeg supported. I
simply copied how they work to be consistent.
Do i:
1. keep it as is and be consistent with the rest?
2. only allow ipfs:// and ipns://?


>
> Also what happens if c->gateway is "ipfs://[...]"? Infinite recursion?
>

Nice one, I need to test that!

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

On Wed, Feb 2, 2022 at 2:29 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Feb 01, 2022 at 10:58:30PM +0100, 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).
> >
> > Signed-off-by: Mark Gaiser <markg85@gmail.com>
> > ---
> >  configure                 |   2 +
> >  doc/protocols.texi        |  30 +++++
> >  libavformat/Makefile      |   2 +
> >  libavformat/ipfsgateway.c | 267 ++++++++++++++++++++++++++++++++++++++
> >  libavformat/protocols.c   |   2 +
> >  5 files changed, 303 insertions(+)
> >  create mode 100644 libavformat/ipfsgateway.c
> >
> > diff --git a/configure b/configure
> > index 5b19a35f59..6ff09e7974 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3585,6 +3585,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 3dc6a479cc..4edce8420f 100644
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -656,6 +656,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..2b52fc9392
> > --- /dev/null
> > +++ b/libavformat/ipfsgateway.c
> > @@ -0,0 +1,267 @@
> > +/*
> > + * 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 "libavutil/avassert.h"
> > +#include "libavutil/avstring.h"
> > +#include "libavutil/internal.h"
> > +#include "libavutil/opt.h"
> > +#include "libavutil/tree.h"
> > +#include "avformat.h"
> > +#include <fcntl.h>
> > +#if HAVE_IO_H
> > +#include <io.h>
> > +#endif
> > +#if HAVE_UNISTD_H
> > +#include <unistd.h>
> > +#endif
> > +#include <sys/stat.h>
> > +#include <stdlib.h>
> > +#include "os_support.h"
> > +#include "url.h"
> > +
>
> > +typedef struct Context {
>
> i also agree that the name is a bit too unspecific
>

Done. changed to IPFSGatewayContext


>
>
> > +    AVClass *class;
> > +    URLContext *inner;
> > +    char *gateway;
> > +} Context;
> > +
> > +// 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.
> > +//
> > +// When done and the return value is 1, a potential IPFS Gateway is set
> in the gateway
> > +// variable in the inner URLContext.
>
> > +static int ff_populate_ipfs_gateway(URLContext *h)
>
> ff_ prefix seems not needed
>

Done. Removed the prefix from both functions.
For the record, I thought it was mandatory to prefix internal functions
with "ff_"?


>
>
> [...]
>
> > +    FILE *gateway_file;
> > +    char gateway_file_data[1000];
>
> [...]
>
> > +    if (!gateway_file) {
> > +        av_log(h, AV_LOG_ERROR, "Unable to open the IPFS gateway file
> (full uri: %s).\n", ipfs_gateway_file);
>
> > +        return -1;
>
> more specific error codes would be better and more consistent to existing
> code
>

Added above the function as:
// Return codes can be:
// 1 : A potential gateway is found and set in c->gateway
// -1: The IPFS data folder could not be found
// -2: The gateway file could not be found
// -3: The gateway file is found but empty
// -9: Unhandled error


>
>
> > +    }
> > +
>
> > +    fscanf(gateway_file, "%[^\n]", gateway_file_data);
>
> how large is the array ?
> how large is the data written into the array ?
> what is the relation between the 2 ?
>

Yeah... I'm going to need more information on that one.
What do you mean?
Is this wrong?


>
> [...]
> > +    int ret;
> > +
> > +    ret = ffurl_read(c->inner, buf, size);
> > +
> > +    return ret;
>
> if thats all that is needed then
>
> return ffurl_read(c->inner, buf, size);
>
> is simpler
>

Ahh nice, fixed for  ipfs_read, ipfs_seek and ipfs_close!

>
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you drop bombs on a foreign country and kill a hundred thousand
> innocent people, expect your government to call the consequence
> "unprovoked inhuman terrorist attacks" and use it to justify dropping
> more bombs and killing more people. The technology changed, the idea is
> old.
> _______________________________________________
> 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".
>

On 02.02.2022 14:56, Mark Gaiser wrote:
> On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen@acc.umu.se> wrote:
> 
>> tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:
>>
>>>
>>> +typedef struct Context {
>>> +    AVClass *class;
>>> +    URLContext *inner;
>>> +    char *gateway;
>>
>> Is there not a maximum length that an HTTP URL can be? At least without
>> query parameters. That way you avoid dynamic allocations. You'd have to
>> separate the AVOption from such a buffer in that case, but I think you
>> have to anyway.
>>
> 
> Could you provide more information on that? Or an example of what you mean
> exactly?
> As far as i know there is no hard limit though it's very much advised to
> not go above 2048 characters.
> 
>>
>>> +    if (!ipfs_full_data_folder) {
>>> +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
>>> +
>>> +        // Try via the home folder.
>>> +        home_folder = getenv("HOME");
>>> +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
>>> home_folder);
>>
>> Memory leak. This applies to most if not all av_asprintf() calls.
>>
> 
> Is there an advised way to neatly clean that up?
> Sure, I can add a bunch of av_free calls to clean it up. But there are
> places where it's not as straightforward like where the av_asprintf was
> done in an if statement. How do I maintain the knowledge that av_asprintf
> was used to call av_free later?
> In a C++ world I'd use a scoped variable ;) But I kinda miss how to do that
> properly here.

You typically make a "goto error" style thing, where you free everything 
that might have been allocated.
Freeing a NULL pointer is valid and does not cause issues, so just 
properly initialize the pointers and av_freep() them on error.

On Wed, Feb 2, 2022 at 3:24 PM Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 02.02.2022 14:56, Mark Gaiser wrote:
> > On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen@acc.umu.se> wrote:
> >
> >> tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:
> >>
> >>>
> >>> +typedef struct Context {
> >>> +    AVClass *class;
> >>> +    URLContext *inner;
> >>> +    char *gateway;
> >>
> >> Is there not a maximum length that an HTTP URL can be? At least without
> >> query parameters. That way you avoid dynamic allocations. You'd have to
> >> separate the AVOption from such a buffer in that case, but I think you
> >> have to anyway.
> >>
> >
> > Could you provide more information on that? Or an example of what you
> mean
> > exactly?
> > As far as i know there is no hard limit though it's very much advised to
> > not go above 2048 characters.
> >
> >>
> >>> +    if (!ipfs_full_data_folder) {
> >>> +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> >>> +
> >>> +        // Try via the home folder.
> >>> +        home_folder = getenv("HOME");
> >>> +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
> >>> home_folder);
> >>
> >> Memory leak. This applies to most if not all av_asprintf() calls.
> >>
> >
> > Is there an advised way to neatly clean that up?
> > Sure, I can add a bunch of av_free calls to clean it up. But there are
> > places where it's not as straightforward like where the av_asprintf was
> > done in an if statement. How do I maintain the knowledge that av_asprintf
> > was used to call av_free later?
> > In a C++ world I'd use a scoped variable ;) But I kinda miss how to do
> that
> > properly here.
>
> You typically make a "goto error" style thing, where you free everything
> that might have been allocated.
> Freeing a NULL pointer is valid and does not cause issues, so just
> properly initialize the pointers and av_freep() them on error.
>

Awesome! Thank you for explaining it, fixing it now.


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

On Wed, Feb 02, 2022 at 03:23:34PM +0100, Mark Gaiser wrote:
> On Wed, Feb 2, 2022 at 2:29 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Tue, Feb 01, 2022 at 10:58:30PM +0100, Mark Gaiser wrote:
[...]
> >
> >
> > > +    AVClass *class;
> > > +    URLContext *inner;
> > > +    char *gateway;
> > > +} Context;
> > > +
> > > +// 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.
> > > +//
> > > +// When done and the return value is 1, a potential IPFS Gateway is set
> > in the gateway
> > > +// variable in the inner URLContext.
> >
> > > +static int ff_populate_ipfs_gateway(URLContext *h)
> >
> > ff_ prefix seems not needed
> >
> 
> Done. Removed the prefix from both functions.
> For the record, I thought it was mandatory to prefix internal functions
> with "ff_"?

only non static ones, these prefixes are to avoid
conflicing with other things in the global namespace


[...]

> 
> 
> >
> >
> > > +    }
> > > +
> >
> > > +    fscanf(gateway_file, "%[^\n]", gateway_file_data);
> >
> > how large is the array ?
> > how large is the data written into the array ?
> > what is the relation between the 2 ?
> >
> 
> Yeah... I'm going to need more information on that one.
> What do you mean?
> Is this wrong?

the array is fixed length, the string written into it is from
some external file IIRC. 
Maybe i missed a check but to me it seemed like the array end can
be overwritten which is bad


thx

[...]

ons 2022-02-02 klockan 14:56 +0100 skrev Mark Gaiser:
> On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen@acc.umu.se>
> wrote:
> 
> > tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:
> > 
> > > 
> > > +typedef struct Context {
> > > +    AVClass *class;
> > > +    URLContext *inner;
> > > +    char *gateway;
> > 
> > Is there not a maximum length that an HTTP URL can be? At least
> > without
> > query parameters. That way you avoid dynamic allocations. You'd
> > have to
> > separate the AVOption from such a buffer in that case, but I think
> > you
> > have to anyway.
> > 
> 
> Could you provide more information on that? Or an example of what you
> mean
> exactly?
> As far as i know there is no hard limit though it's very much advised
> to
> not go above 2048 characters.

You could at least separate the variable that is set by the AVOption
stuff from the buffer that's produced by the URL parsing stuff.

Since you call ffurl_open_whitelist() immediately you could just use
alloca() to allocate memory on the stack. That way you don't have to
worry about deallocation at all *and* you can have it be arbitrarily
large.

If URLs have a spec'd maximum length then a buffer of constant size on
the stack would work.

> 
> > 
> > > +    if (!ipfs_full_data_folder) {
> > > +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> > > +
> > > +        // Try via the home folder.
> > > +        home_folder = getenv("HOME");
> > > +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
> > > home_folder);
> > 
> > Memory leak. This applies to most if not all av_asprintf() calls.
> > 
> 
> Is there an advised way to neatly clean that up?
> Sure, I can add a bunch of av_free calls to clean it up. But there
> are
> places where it's not as straightforward like where the av_asprintf
> was
> done in an if statement. How do I maintain the knowledge that
> av_asprintf
> was used to call av_free later?

goto as the others pointed out


> 
> > 
> > > +static int translate_ipfs_to_http(URLContext *h, const char
> > > *uri,
> > > int flags, AVDictionary **options)
> > > +{
> > > +    const char *ipfs_cid;
> > > +    const char *protocol_path_suffix = "ipfs/";
> > > +    char *fulluri;
> > > +    int ret;
> > > +    Context *c = h->priv_data;
> > > +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> > > av_strstart(uri, "ipfs:", &ipfs_cid));
> > > +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> > > av_strstart(uri, "ipns:", &ipfs_cid));
> > 
> > https://docs.ipfs.io/concepts/ipfs-gateway/ claims ipfs:// is the
> > canonical form. No mentioned is made of any ipfs:{CID} form.
> > Incorrect
> > URLs should be rejected, not silently patched.
> > 
> 
> I'd like to make a decision here. This current logic (ipfs:// and
> ipfs:,
> same for ipns) is inspired by other protocols that ffmpeg supported.
> I
> simply copied how they work to be consistent.
> Do i:
> 1. keep it as is and be consistent with the rest?
> 2. only allow ipfs:// and ipns://?
> 

Which protocols? This laxness in lavf is worrisome. It breeds laxness
in other projects.

/Tomas

On Fri, Feb 4, 2022 at 11:29 AM Tomas Härdin <tjoppen@acc.umu.se> wrote:

> ons 2022-02-02 klockan 14:56 +0100 skrev Mark Gaiser:
> > On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen@acc.umu.se>
> > wrote:
> >
> > > tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:
> > >
> > > >
> > > > +typedef struct Context {
> > > > +    AVClass *class;
> > > > +    URLContext *inner;
> > > > +    char *gateway;
> > >
> > > Is there not a maximum length that an HTTP URL can be? At least
> > > without
> > > query parameters. That way you avoid dynamic allocations. You'd
> > > have to
> > > separate the AVOption from such a buffer in that case, but I think
> > > you
> > > have to anyway.
> > >
> >
> > Could you provide more information on that? Or an example of what you
> > mean
> > exactly?
> > As far as i know there is no hard limit though it's very much advised
> > to
> > not go above 2048 characters.
>
> You could at least separate the variable that is set by the AVOption
> stuff from the buffer that's produced by the URL parsing stuff.
>
> Since you call ffurl_open_whitelist() immediately you could just use
> alloca() to allocate memory on the stack. That way you don't have to
> worry about deallocation at all *and* you can have it be arbitrarily
> large.
>
> If URLs have a spec'd maximum length then a buffer of constant size on
> the stack would work.
>
> >
> > >
> > > > +    if (!ipfs_full_data_folder) {
> > > > +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> > > > +
> > > > +        // Try via the home folder.
> > > > +        home_folder = getenv("HOME");
> > > > +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
> > > > home_folder);
> > >
> > > Memory leak. This applies to most if not all av_asprintf() calls.
> > >
> >
> > Is there an advised way to neatly clean that up?
> > Sure, I can add a bunch of av_free calls to clean it up. But there
> > are
> > places where it's not as straightforward like where the av_asprintf
> > was
> > done in an if statement. How do I maintain the knowledge that
> > av_asprintf
> > was used to call av_free later?
>
> goto as the others pointed out
>
>
> >
> > >
> > > > +static int translate_ipfs_to_http(URLContext *h, const char
> > > > *uri,
> > > > int flags, AVDictionary **options)
> > > > +{
> > > > +    const char *ipfs_cid;
> > > > +    const char *protocol_path_suffix = "ipfs/";
> > > > +    char *fulluri;
> > > > +    int ret;
> > > > +    Context *c = h->priv_data;
> > > > +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> > > > av_strstart(uri, "ipfs:", &ipfs_cid));
> > > > +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> > > > av_strstart(uri, "ipns:", &ipfs_cid));
> > >
> > > https://docs.ipfs.io/concepts/ipfs-gateway/ claims ipfs:// is the
> > > canonical form. No mentioned is made of any ipfs:{CID} form.
> > > Incorrect
> > > URLs should be rejected, not silently patched.
> > >
> >
> > I'd like to make a decision here. This current logic (ipfs:// and
> > ipfs:,
> > same for ipns) is inspired by other protocols that ffmpeg supported.
> > I
> > simply copied how they work to be consistent.
> > Do i:
> > 1. keep it as is and be consistent with the rest?
> > 2. only allow ipfs:// and ipns://?
> >
>
> Which protocols? This laxness in lavf is worrisome. It breeds laxness
> in other projects.
>

Hmm, I might have been looking at the wrong example and calling that
"others do it too".
Crypto does it with "crypto:" and "crypto+"

From a quick glance that's about it.

I'll just remove "ipfs:". I only wanted to stay consistent with what I knew
(from crypto) but that is apparently inconsistent.



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