On Thu, 12 Apr 2012, Samuel Pitoiset wrote:
On Thu, Apr 12, 2012 at 6:18 PM, Samuel Pitoiset <[email protected]>
wrote:
---------- Forwarded message ----------
From: Samuel Pitoiset <[email protected]>
Date: Thu, Apr 12, 2012 at 6:17 PM
Subject: Re: [libav-devel] [PATCH] rtmp: Support 'app', an
option which overrides the name of application [GSoC]
To: Anton Khirnov <[email protected]>
On Thu, Apr 12, 2012 at 8:00 AM, Anton Khirnov
<[email protected]> wrote:
On Mon, 9 Apr 2012 00:51:26 +0200, Samuel Pitoiset
<[email protected]> wrote:
> Sometimes the URL parser cannot determine the app
name automatically,
> so it must be given explicitly using this option
(ie. -app).
> ---
> libavformat/rtmpproto.c | 40
+++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/rtmpproto.c
b/libavformat/rtmpproto.c
> index 7683559..be82f0f 100644
> --- a/libavformat/rtmpproto.c
> +++ b/libavformat/rtmpproto.c
> @@ -28,6 +28,7 @@
> #include "libavutil/avstring.h"
> #include "libavutil/intfloat.h"
> #include "libavutil/lfg.h"
> +#include "libavutil/opt.h"
> #include "libavutil/sha.h"
> #include "avformat.h"
> #include "internal.h"
> @@ -41,6 +42,8 @@
>
> //#define DEBUG
>
> +#define APP_MAX_LENGTH 128
> +
> /** RTMP protocol handler state */
> typedef enum {
> STATE_START, ///< client has not done
anything yet
> @@ -56,12 +59,13 @@ typedef enum {
>
> /** protocol handler context */
> typedef struct RTMPContext {
> + const AVClass *class;
> URLContext* stream;
///< TCP stream used in interactions with RTMP
server
> RTMPPacket prev_pkt[2][RTMP_CHANNELS];
///< packet history used when reading and sending
packets
> int chunk_size;
///< size of the chunks RTMP packets are divided
into
> int is_input;
///< input/output flag
> char playpath[256];
///< path to filename to play (with possible "mp4:"
prefix)
> - char app[128];
///< application
> + char *app;
///< name of application
> ClientState state;
///< current state
> int main_channel_id;
///< an additional channel ID which is used for
some invocations
> uint8_t* flv_data;
///< buffer with data for demuxer
> @@ -822,6 +826,7 @@ static int
rtmp_open(URLContext *s, const char *uri, int flags)
> {
> RTMPContext *rt = s->priv_data;
> char proto[8], hostname[256], path[1024],
*fname;
> + char *old_app;
> uint8_t buf[2048];
> int port;
> int ret;
> @@ -847,6 +852,16 @@ static int
rtmp_open(URLContext *s, const char *uri, int flags)
>
> rt->chunk_size = 128;
> rt->state = STATE_HANDSHAKED;
> +
> + // Keep the application name when it has been
defined by the user.
> + old_app = rt->app ? av_strdup(rt->app) :
NULL;
> +
> + rt->app = av_malloc(APP_MAX_LENGTH);
Aren't you leaking rt->app here?
Strings set by AVOptions are malloced, there's no need for
strdup I
think.
No, there is no memleaks here.
Did you try it in valgrind? When I try, I get leaks here.
And Martin Storsjö said "IIRC this shouldn't be needed, if you've
declared it as an option, the normal option handling logic should free
it (regardless if you've set it via an option or set by the protocol
code itself)."
No, that wasn't related to these particular lines. I meant that you don't
need to free the app pointer in the rtmp_close function, since the option
freeing logic will free it.
I have to store the old value of the application name when it has been
defined by the user, so strdup() is required.
No - if the string is set here already, it is an allocated string (with no
other references to it than this), so if you strdup it, you will have two
allocations of the same and leak the first one.
Bonus issue to fix: Earlier, you'd have to specify everything as
rtmp://server/<app>/<playpath>. Now you can do e.g. avconv -application
<app> -playpath <playpath> -i rtmp://server/.
If you instead of -i rtmp://server/ just do -i rtmp://server, the code
will use uninitialized memory. A fix for that is also welcome. :-)
Also, I'd much rather have the parameter named rtmp_app instead of the
overly generic app/application, I think all others also agreed on that.
We've got the option rtsp_transport for rtsp, too, since it would feel
overly generic without the prefix.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
