rtmp: Display an error message when the stream is not found

Message ID 1339519008-17273-1-git-send-email-samuel.pitoiset@gmail.com
State Superseded
Headers show

Commit Message

Samuel Pitoiset June 12, 2012, 4:36 p.m.
The rtmp_live option must be set by default in recorded mode for
displaying that error message. This fixes bugzilla bug #307.
---
 libavformat/rtmpproto.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Luca Barbato June 12, 2012, 4:47 p.m. | #1
On 06/12/2012 06:36 PM, Samuel Pitoiset wrote:
> The rtmp_live option must be set by default in recorded mode for
> displaying that error message. This fixes bugzilla bug #307.
> ---
>  libavformat/rtmpproto.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Seems that the patch and the commit message mismatch, why setting it to
on-demand?

lu
Martin Storsjö June 12, 2012, 7:41 p.m. | #2
On Tue, 12 Jun 2012, Samuel Pitoiset wrote:

> The rtmp_live option must be set by default in recorded mode for
> displaying that error message. This fixes bugzilla bug #307.
> ---
> libavformat/rtmpproto.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
> index 56011f1..98e483f 100644
> --- a/libavformat/rtmpproto.c
> +++ b/libavformat/rtmpproto.c
> @@ -1330,7 +1330,7 @@ static const AVOption rtmp_options[] = {
>     {"rtmp_app", "Name of application to connect to on the RTMP server", OFFSET(app), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
>     {"rtmp_conn", "Append arbitrary AMF data to the Connect message", OFFSET(conn), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
>     {"rtmp_flashver", "Version of the Flash plugin used to run the SWF player.", OFFSET(flashver), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
> -    {"rtmp_live", "Specify that the media is a live stream.", OFFSET(live), AV_OPT_TYPE_INT, {-2}, INT_MIN, INT_MAX, DEC, "rtmp_live"},
> +    {"rtmp_live", "Specify that the media is a live stream.", OFFSET(live), AV_OPT_TYPE_INT, {0}, INT_MIN, INT_MAX, DEC, "rtmp_live"},
>     {"any", "both", 0, AV_OPT_TYPE_CONST, {-2}, 0, 0, DEC, "rtmp_live"},
>     {"live", "live stream", 0, AV_OPT_TYPE_CONST, {-1}, 0, 0, DEC, "rtmp_live"},
>     {"recorded", "recorded stream", 0, AV_OPT_TYPE_CONST, {0}, 0, 0, DEC, "rtmp_live"},
> -- 
> 1.7.10.3

As Luca said, the commit description and actual change don't seem to match 
at all.

// Martin
Martin Storsjö June 12, 2012, 8:03 p.m. | #3
On Tue, 12 Jun 2012, Martin Storsjö wrote:

> On Tue, 12 Jun 2012, Samuel Pitoiset wrote:
>
>> The rtmp_live option must be set by default in recorded mode for
>> displaying that error message. This fixes bugzilla bug #307.
>> ---
>> libavformat/rtmpproto.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
>> index 56011f1..98e483f 100644
>> --- a/libavformat/rtmpproto.c
>> +++ b/libavformat/rtmpproto.c
>> @@ -1330,7 +1330,7 @@ static const AVOption rtmp_options[] = {
>>     {"rtmp_app", "Name of application to connect to on the RTMP server", 
>> OFFSET(app), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
>>     {"rtmp_conn", "Append arbitrary AMF data to the Connect message", 
>> OFFSET(conn), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
>>     {"rtmp_flashver", "Version of the Flash plugin used to run the SWF 
>> player.", OFFSET(flashver), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, 
>> DEC|ENC},
>> -    {"rtmp_live", "Specify that the media is a live stream.", 
>> OFFSET(live), AV_OPT_TYPE_INT, {-2}, INT_MIN, INT_MAX, DEC, "rtmp_live"},
>> +    {"rtmp_live", "Specify that the media is a live stream.", 
>> OFFSET(live), AV_OPT_TYPE_INT, {0}, INT_MIN, INT_MAX, DEC, "rtmp_live"},
>>     {"any", "both", 0, AV_OPT_TYPE_CONST, {-2}, 0, 0, DEC, "rtmp_live"},
>>     {"live", "live stream", 0, AV_OPT_TYPE_CONST, {-1}, 0, 0, DEC, 
>> "rtmp_live"},
>>     {"recorded", "recorded stream", 0, AV_OPT_TYPE_CONST, {0}, 0, 0, DEC, 
>> "rtmp_live"},
>> -- 
>> 1.7.10.3
>
> As Luca said, the commit description and actual change don't seem to match at 
> all.

So apparently this actually does the thing you describe, but you don't 
really describe why changing this default makes an error be printed. We 
also just recently added docs saying that the default is "any", so you'd 
have update that accordingly.

And is this just a case of trading one thing for another, getting better 
error messages in some cases but breaking stuff that used to work (live 
streams that did not require any parameters to be played before, when the 
default was "any")? You need to take all of that into consideration. And 
if/when changing a default which breaks things like this, one has to say 
it openly and be ready to argue why it is a tradeoff worth making.

// Martin
Luca Barbato June 12, 2012, 10:11 p.m. | #4
On 06/12/2012 10:03 PM, Martin Storsjö wrote:
> So apparently this actually does the thing you describe, but you don't
> really describe why changing this default makes an error be printed. We
> also just recently added docs saying that the default is "any", so you'd
> have update that accordingly.

And I'd keep the current situation as is.

lu
Martin Storsjö June 13, 2012, 7:17 a.m. | #5
On Wed, 13 Jun 2012, Luca Barbato wrote:

> On 06/12/2012 10:03 PM, Martin Storsjö wrote:
>> So apparently this actually does the thing you describe, but you don't
>> really describe why changing this default makes an error be printed. We
>> also just recently added docs saying that the default is "any", so you'd
>> have update that accordingly.
>
> And I'd keep the current situation as is.

Yes, at least until there's a proper explanation of why this would be a 
better solution in general and not just for this particular stream.

// Martin
Samuel Pitoiset June 13, 2012, 2:44 p.m. | #6
On Wed, Jun 13, 2012 at 9:17 AM, Martin Storsjö <martin@martin.st> wrote:
> On Wed, 13 Jun 2012, Luca Barbato wrote:
>
>> On 06/12/2012 10:03 PM, Martin Storsjö wrote:
>>>
>>> So apparently this actually does the thing you describe, but you don't
>>> really describe why changing this default makes an error be printed. We
>>> also just recently added docs saying that the default is "any", so you'd
>>> have update that accordingly.
>>
>>
>> And I'd keep the current situation as is.
>
>
> Yes, at least until there's a proper explanation of why this would be a
> better solution in general and not just for this particular stream.

Actually, it's also the default value of rtmpdump, so, I guess that
won't break anything.
Martin Storsjö June 13, 2012, 6:50 p.m. | #7
On Wed, 13 Jun 2012, Samuel Pitoiset wrote:

> On Wed, Jun 13, 2012 at 9:17 AM, Martin Storsjö <martin@martin.st> wrote:
>> On Wed, 13 Jun 2012, Luca Barbato wrote:
>>
>>> On 06/12/2012 10:03 PM, Martin Storsjö wrote:
>>>>
>>>> So apparently this actually does the thing you describe, but you don't
>>>> really describe why changing this default makes an error be printed. We
>>>> also just recently added docs saying that the default is "any", so you'd
>>>> have update that accordingly.
>>>
>>>
>>> And I'd keep the current situation as is.
>>
>>
>> Yes, at least until there's a proper explanation of why this would be a
>> better solution in general and not just for this particular stream.
>
> Actually, it's also the default value of rtmpdump, so, I guess that
> won't break anything.

No - there are streams that play well with the current default of "any", 
but which fail to play with "recorded". If you switch the default, this 
setup will break for those users (until they add the parameter explicitly 
of course) - changing the default behaviour always brings some sort of 
breakage which has to be weighed against the benefit

There can quite possibly exist other servers that will have the exact same 
issue but in the other direction, so if you use the mode "recorded", it 
would not return any error message for "stream not found". And then 
somebody would file a bug for that. And then we would have to switch the 
default back again.

That's why I'm against this "fix", since it's just a tweak which makes one 
case better and other cases worse.

Also, if this particular server behaves like this for the "any"/-2 
parameter, it's IMO broken. If we can easily accommodate such servers, 
fine, but this particular fix just trades one issue for another.

Whether this is the default value for rtmpdump or not doesn't matter much 
- that's no authority on these things, only a useful reference. The 
official flash player is more of a authority of default behaviour, and 
there, the default value of this parameter is -2.

// Martin
Samuel Pitoiset June 13, 2012, 8:40 p.m. | #8
On Wed, Jun 13, 2012 at 8:50 PM, Martin Storsjö <martin@martin.st> wrote:
> On Wed, 13 Jun 2012, Samuel Pitoiset wrote:
>
>> On Wed, Jun 13, 2012 at 9:17 AM, Martin Storsjö <martin@martin.st> wrote:
>>>
>>> On Wed, 13 Jun 2012, Luca Barbato wrote:
>>>
>>>> On 06/12/2012 10:03 PM, Martin Storsjö wrote:
>>>>>
>>>>>
>>>>> So apparently this actually does the thing you describe, but you don't
>>>>> really describe why changing this default makes an error be printed. We
>>>>> also just recently added docs saying that the default is "any", so
>>>>> you'd
>>>>> have update that accordingly.
>>>>
>>>>
>>>>
>>>> And I'd keep the current situation as is.
>>>
>>>
>>>
>>> Yes, at least until there's a proper explanation of why this would be a
>>> better solution in general and not just for this particular stream.
>>
>>
>> Actually, it's also the default value of rtmpdump, so, I guess that
>> won't break anything.
>
>
> No - there are streams that play well with the current default of "any", but
> which fail to play with "recorded". If you switch the default, this setup
> will break for those users (until they add the parameter explicitly of
> course) - changing the default behaviour always brings some sort of breakage
> which has to be weighed against the benefit
>
> There can quite possibly exist other servers that will have the exact same
> issue but in the other direction, so if you use the mode "recorded", it
> would not return any error message for "stream not found". And then somebody
> would file a bug for that. And then we would have to switch the default back
> again.
>
> That's why I'm against this "fix", since it's just a tweak which makes one
> case better and other cases worse.
>
> Also, if this particular server behaves like this for the "any"/-2
> parameter, it's IMO broken. If we can easily accommodate such servers, fine,
> but this particular fix just trades one issue for another.
>
> Whether this is the default value for rtmpdump or not doesn't matter much -
> that's no authority on these things, only a useful reference. The official
> flash player is more of a authority of default behaviour, and there, the
> default value of this parameter is -2.

Okay, I agree with you. Let's me discard it.

Patch

diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
index 56011f1..98e483f 100644
--- a/libavformat/rtmpproto.c
+++ b/libavformat/rtmpproto.c
@@ -1330,7 +1330,7 @@  static const AVOption rtmp_options[] = {
     {"rtmp_app", "Name of application to connect to on the RTMP server", OFFSET(app), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
     {"rtmp_conn", "Append arbitrary AMF data to the Connect message", OFFSET(conn), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
     {"rtmp_flashver", "Version of the Flash plugin used to run the SWF player.", OFFSET(flashver), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
-    {"rtmp_live", "Specify that the media is a live stream.", OFFSET(live), AV_OPT_TYPE_INT, {-2}, INT_MIN, INT_MAX, DEC, "rtmp_live"},
+    {"rtmp_live", "Specify that the media is a live stream.", OFFSET(live), AV_OPT_TYPE_INT, {0}, INT_MIN, INT_MAX, DEC, "rtmp_live"},
     {"any", "both", 0, AV_OPT_TYPE_CONST, {-2}, 0, 0, DEC, "rtmp_live"},
     {"live", "live stream", 0, AV_OPT_TYPE_CONST, {-1}, 0, 0, DEC, "rtmp_live"},
     {"recorded", "recorded stream", 0, AV_OPT_TYPE_CONST, {0}, 0, 0, DEC, "rtmp_live"},