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

[Mark Gaiser <markg85@gmail.com>]

On Mon, Mar 28, 2022 at 6:44 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Mar 28, 2022 at 06:34:33PM +0200, Mark Gaiser wrote:
> > On Mon, Mar 28, 2022 at 6:19 PM Michael Niedermayer <
> michael@niedermayer.cc>
> > wrote:
> >
> > > On Fri, Mar 18, 2022 at 03:50:05PM +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 | 310
> ++++++++++++++++++++++++++++++++++++++
> > > >  libavformat/protocols.c   |   2 +
> > > >  5 files changed, 346 insertions(+)
> > > >  create mode 100644 libavformat/ipfsgateway.c
> > >
> > > Theres some trailing whitespace which needs to be removed
> > > our git scripts block trailing whitespace in most files
> > >
> > > [...]
> > > > +static int ipfs_close(URLContext *h)
> > > > +{
> > > > +    IPFSGatewayContext *c = h->priv_data;
> > > > +    av_free(c->gateway);
> > >
> > > this results in a double free
> > >
> >
> > I believe one of the earlier feedback rounds told me to put it here.
> > It's not free'd anywhere else.
>
> ==22837== Invalid free() / delete / delete[] / realloc()
> ==22837==    at 0x4C32D3B: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==22837==    by 0x117FF88: av_opt_free (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5C58CF: ffurl_closep (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5C5AF2: ffurl_close (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5CA206: avio_close (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x3021C0: ffmpeg_cleanup (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x2F5FA0: exit_program (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x2E34A1: main (in ffmpeg/ffmpeg_g)
> ==22837==  Address 0x2ced8760 is 0 bytes inside a block of size 17 free'd
> ==22837==    at 0x4C32D3B: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==22837==    by 0x7372BD: ipfs_close (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5C588C: ffurl_closep (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5C5AF2: ffurl_close (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5CA206: avio_close (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x3021C0: ffmpeg_cleanup (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x2F5FA0: exit_program (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x2E34A1: main (in ffmpeg/ffmpeg_g)
> ==22837==  Block was alloc'd at
> ==22837==    at 0x4C31A3F: malloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==22837==    by 0x4C33D84: realloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==22837==    by 0x117E120: av_strdup (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x1181DEF: av_opt_set (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x118272D: av_opt_set_dict2 (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5C598C: ffurl_open_whitelist (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5CA4DD: ffio_open_whitelist (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x6B88FB: io_open_default (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x5E0E9E: avformat_open_input (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x2EB32B: open_input_file (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x2EF00B: ffmpeg_parse_options (in ffmpeg/ffmpeg_g)
> ==22837==    by 0x2E3301: main (in ffmpeg/ffmpeg_g)
>
>
> >
> > Then again, in those earlier rounds I was manipulating c-gateway which
> > right now isn't the case at all anymore.
> >
> > If all that's stopping it from merging is this single line, could you
> > perhaps merge it and remove this line while at it?
> > I'm kinda reluctant to make another patch and wait 1-2 weeks again...
>
> I do not know why its there or who asked for it to be put there.
> I dont want to just remove something while merging that someone else
> asked to be added
>

It's fine, you can remove it.
I just checked with crypto.c (which I use as an example).

d->gateway is an AVOption and is never changed in code.
If I compare it with other AVOption in crypto.c, it too follows the same
concept of never deleting it manually (probably AVOption magic internally?)

Removing it is fine. Unless crypto.c does it wrong too ;)
Could you?
Or do you insist on an updated patch? I would merely remove that one line..


>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Never trust a computer, one day, it may think you are the virus. -- Compn
> _______________________________________________
> 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".