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

[Nicolas Adenis-Lamarre]

>> Any I/O can cause delay. When the user close the window, they want it to
>> close immediately, not in five seconds.
Can you confirm that the problem is blocking io, and more generally
blocking system calls more than just io ? (i mean, we must take no risk to
have to wait, versus doing some non blocking io)
tearing is just a packet to send. No answer has to be waited. And if it's
not done, it's not dramatic, it's just better.
Sending the message in a non blocking manner could be accepted by ffmpeg
team ? I thing it's a very short io, sending just to the linux net stack a
packet. (no network wait). It seems to fail only if the net stack is
already filled as far as i understand.

http://linux.die.net/man/2/send
*: send man pageMSG_DONTWAIT* (since Linux 2.2) Enables nonblocking
operation; if the operation would block, *EAGAIN* or *EWOULDBLOCK* is
returned (this can also be enabled using the *O_NONBLOCK* flag with
the *F_SETFL
fcntl*(2)).)

I don't understand why the close must be done before the windows closure.
Why not closing the window first, then close the protocol (but being sure
it will not stuck for ever).

>> if the server does not take it into account, blame it for being stupid.
I agree the server is stupid. However, if one does, i guess it's based on a
software available to many places.
And the server programmers would tell i don't respect the protocol. But i
definitively agree with that point.

>> All this is necessary because there are various causes for blocking; if
>> ffplay is stuck waiting for more data in the middle of a packet header
but
>> the network went down, interrupting the I/O is the only solution.
tearing packet is not a packet for which you have to wait something in
feedback. (it's only a write ; and i think it's possible to make it non
blocking generating an EAGAIN to ignore or to test a second time before
giving up.)

Nicolas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Sun, Oct 18, 2015 at 10:13:29PM +0200, Nicolas Adenis-Lamarre wrote:
> The rtsp protocol requires the client to send a packet at the end of the
> connexion.
> FFmpeg basic network function check wether the user aborted the
> communication and don't send the packet in this case.
> So the protocol is not respected.
> This commit removes the check. An other possibility would have to add an
> extra parameter to these functions to force the packet sending.

When the rt*p code and the connection is correctly functioning
then ff_check_interrupt() will return 0
When the code gets stuck due to the network connection or other
side failing then ff_check_interrupt() can return non zero if the
user application wants to get control back in a last ditch effort
before killing the process or thread.

Now i do no know enough to say what the problem is exactly
maybe this is a bug in the user application and it simply should not
use the interrupt callback to terminate things
or maybe the application has a good reason why it needs to use this
method, in which case maybe the API is too limited to indicate that
clean termination is requested instead of imedeate abortion of all
transfers.
If this is due to limitations of the API then extending the API is
welcome but it should be done in a way that doesnt break existing
user applications
I belive the patch here would break the case where a actual hard and
interruption is wanted because maybe all network communication would
timeout due to network being down or such

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Nicolas Adenis-Lamarre]

Let's take the example of ffplay in which the code always fails.
ie : ffplay 'rtsp://
mafreebox.freebox.fr/fbxtv_pub/stream?namespace=1=201=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(>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.

Nicolas



2015-10-20 15:50 GMT+02:00 Michael Niedermayer :

> On Sun, Oct 18, 2015 at 10:13:29PM +0200, Nicolas Adenis-Lamarre wrote:
> > The rtsp protocol requires the client to send a packet at the end of the
> > connexion.
> > FFmpeg basic network function check wether the user aborted the
> > communication and don't send the packet in this case.
> > So the protocol is not respected.
> > This commit removes the check. An other possibility would have to add an
> > extra parameter to these functions to force the packet sending.
>
> When the rt*p code and the connection is correctly functioning
> then ff_check_interrupt() will return 0
> When the code gets stuck due to the network connection or other
> side failing then ff_check_interrupt() can return non zero if the
> user application wants to get control back in a last ditch effort
> before killing the process or thread.
>
> Now i do no know enough to say what the problem is exactly
> maybe this is a bug in the user application and it simply should not
> use the interrupt callback to terminate things
> or maybe the application has a good reason why it needs to use this
> method, in which case maybe the API is too limited to indicate that
> clean termination is requested instead of imedeate abortion of all
> transfers.
> If this is due to limitations of the API then extending the API is
> welcome but it should be done in a way that doesnt break existing
> user applications
> I belive the patch here would break the case where a actual hard and
> interruption is wanted because maybe all network communication would
> timeout due to network being down or such
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> it is not once nor twice but times without number that the same ideas make
> their appearance in the world. -- Aristotle
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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=201=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(>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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Nicolas George]

Le nonidi 29 vendémiaire, an CCXXIV, Nicolas Adenis-Lamarre a écrit :
> 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.

Any I/O can cause delay. When the user close the window, they want it to
close immediately, not in five seconds. Even a tenth of a second gives a
noticeable lag, it is not annoying in itself bug will cause the user to
unconsciously consider the program badly finished.

> 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)

Look at the code:

static void stream_close(VideoState *is)
{
/* XXX: use a special url_shutdown call to abort parse cleanly */
is->abort_request = 1;
SDL_WaitThread(is->read_tid, NULL);

The author was well aware of the issue, but interrupting the network flow is
simpler.

Note that it should not matter: the teardown packet is required by the
protocol, but the closure is also transmitted by other means (probably TCP
RST or ICMP, depending on the transport layer used), and if the server does
not take it into account, blame it for being stupid.

> * 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.

As you can see, a correct implementation of the shutdown would be
appreciated. IMHO, the best way of doing so would be:

- queue an immediate display of having taken the user's request into
  account;

- start clean shutdown;

- wait a configurable amount of time and revert to interrupting the I/O.

All this is necessary because there are various causes for blocking; if
ffplay is stuck waiting for more data in the middle of a packet header but
the network went down, interrupting the I/O is the only solution.

> 2015-10-20 19:20 GMT+02:00 Michael Niedermayer :

Please remember not to top-post on this list; if you do not know what it
means, look it up.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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 :

> 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=201=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(>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 

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

[Nicolas Adenis-Lamarre]

The rtsp protocol requires the client to send a packet at the end of the
connexion.
FFmpeg basic network function check wether the user aborted the
communication and don't send the packet in this case.
So the protocol is not respected.
This commit removes the check. An other possibility would have to add an
extra parameter to these functions to force the packet sending.

https://trac.ffmpeg.org/ticket/4929

Signed-off-by: Nicolas Adenis-Lamarre 
From 415ea8cad8cd4df9c32229918ccb0c84ca2c1c4a Mon Sep 17 00:00:00 2001
From: Nicolas Adenis-Lamarre 
Date: Sun, 18 Oct 2015 22:02:47 +0200
Subject: [PATCH] rtsp protocol : teardown packet not sent

The rtsp protocol requires the client to send a packet at the end of the connexion.
FFmpeg basic network function check wether the user aborted the communication and don't send the packet in this case.
So the protocol is not respected.
This commit removes the check. An other possibility would have to add an extra parameter to these functions to force the packet sending.

Signed-off-by: Nicolas Adenis-Lamarre 
---
 libavformat/avio.c| 2 --
 libavformat/network.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/libavformat/avio.c b/libavformat/avio.c
index 21713d9..523947f 100644
--- a/libavformat/avio.c
+++ b/libavformat/avio.c
@@ -321,8 +321,6 @@ static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf,
 
 len = 0;
 while (len < size_min) {
-if (ff_check_interrupt(>interrupt_callback))
-return AVERROR_EXIT;
 ret = transfer_func(h, buf + len, size - len);
 if (ret == AVERROR(EINTR))
 continue;
diff --git a/libavformat/network.c b/libavformat/network.c
index 7a326d2..fa00ce8 100644
--- a/libavformat/network.c
+++ b/libavformat/network.c
@@ -85,8 +85,6 @@ int ff_network_wait_fd_timeout(int fd, int write, int64_t timeout, AVIOInterrupt
 int64_t wait_start = 0;
 
 while (1) {
-if (ff_check_interrupt(int_cb))
-return AVERROR_EXIT;
 ret = ff_network_wait_fd(fd, write);
 if (ret != AVERROR(EAGAIN))
 return ret;
-- 
2.1.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel