[FFmpeg-devel] [PATCH] rtsp protocol : teardown packet not sent

[Nicolas Adenis-Lamarre]

Thanks for your answer.
Thanks for your light. It's clear that my patch is wrong (even if it makes
ffplay working).
I've 2 solutions at the end, i hope that one of them could be valid for you.

For the moment, i still don't know what is the best way to fix it.

Probably i misunderstood again, but,
I don't understand however in which case an application would use a
callback to avoid any io at all.
For example, i don't understand why ffplay, when a user close the window,
choose to avoid any io at all.

in ffplay.c, would it be ok to change
static int decode_interrupt_cb(void *ctx) { return ctx->abort_request; }
by
static int decode_interrupt_cb(void *ctx) { return 0; }
?
(i don't think so)

* extend/improve/fix the API => hard to me to say if that's the good way
for the moment
"Callback for checking whether to abort blocking functions."
What i understand, is that ffmpeg has a callback(ff_check_interrupt) saying
that at some point, no more io must be done.

* change the user application to not return 1 from the callback
=> i could, but i'm afraid that even ffplay which is a part of ffmpeg has
the problem.
So, i guess it's not the only application to have the problem (kodi has
too, while it's my target).
More over, i don't think that application must known that in some case,
close must do io.

* not just copy the callback for the demuxer to the protocol used for
  tearing down the connection while still by some means ensuring that
  a user request for interrupting a possibly blocking demuxer is
  fullfilled
=> i think it would not respect the protocol, while it would bypass the io
callback.

* There are probably other options, but the documented API must match
  the implementation and should be sane and also not do
  unexpected things to existing implementations writen based on the
  current API
=> I agree

>>> so, because i would like to fix it, i suggest 2 things, tell me if one
of them could be ok or both have no chance to be validated :
1) as you suggested, bypass the check in the case of rtsp. It's not perfect
and i don't really like it, but i would make things working.

2) add an extra argument to tell the io is important or not and change the
api so that the blocking callback is only on non important io
by important, it could be protocol satisfaction, versus packet not
mandatory / not required / to abort if it is not so important.

Nicolas


2015-10-20 19:20 GMT+02:00 Michael Niedermayer <michael at niedermayer.cc>:

> On Tue, Oct 20, 2015 at 06:16:12PM +0200, Nicolas Adenis-Lamarre wrote:
> > Let's take the example of ffplay in which the code always fails.
> > ie : ffplay 'rtsp://
> > mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1&service=201&flavour=ld
> '
> > fails to respect the protocol in 100% of the cases.
> >
> > When i close the window,
> > ffplay.c : stream_close() is called
> > => is->abort_request = 1;
> > => avformat_close_input(&is->ic);
> >
> > then
> > rstpdec.c : rtsp_read_close(AVFormatContext *s)
> > => ff_rtsp_send_cmd_async(s, "TEARDOWN", rt->control_uri, NULL);
> > => this call always fails because it calls sub routines like
> > network.c:ff_network_wait_fd_timeout which fails if interrupt is done.
> >
> > In my humble opinion, network.c:ff_network_wait_fd_timeout should not
> > decicde to not send a packet because interrupt is done,
> > it makes it impossible to send a packet after the interrup is done, which
> > makes impossible to respect the rtsp protocol.
> > In my humble opinion, the choice should be done in the upper stacks.
> >
> > This is why i made this patch.
> > An other suggestion if you fear to break something, is to add an extra
> > parameter called "ignore_interrupt", to add a check, but in the
> absolute, i
> > think the choice in my patch is preferable, even it could break things at
> > the beginning, the time, people find where to add correctly the interrupt
> > check at the good place, not in the low stack.
>
>
> avio.h:
>     /**
>     * Callback for checking whether to abort blocking functions.
>     * AVERROR_EXIT is returned in this case by the interrupted
>     * function. During blocking operations, callback is called with
>     * opaque as parameter. If the callback returns 1, the
>     * blocking operation will be aborted.
>     *
>     * No members can be added to this struct without a major bump, if
>     * new elements have been added after this struct in AVFormatContext
>     * or AVIOContext.
>     */
>     typedef struct AVIOInterruptCB {
>
> Thus if the callback is set for the protocol and if it returns 1
> the fuction must abort if its (potentially) blocking. That also is
> true for any teardown operations because they can block too.
>
> you cannot just ignore the API as it is documented and disable
> the interrupt callback machanism more or less entirely
>
> What you can do is,
> * extend/improve/fix the API
> * change the user application to not return 1 from the callback
> * not just copy the callback for the demuxer to the protocol used for
>   tearing down the connection while still by some means ensuring that
>   a user request for interrupting a possibly blocking demuxer is
>   fullfilled
> * There are probably other options, but the documented API must match
>   the implementation and should be sane and also not do
>   unexpected things to existing implementations writen based on the
>   current API
>
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I am the wisest man alive, for I know one thing, and that is that I know
> nothing. -- Socrates
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>