On Thu, Apr 12, 2012 at 8:53 PM, Martin Storsjö <[email protected]> wrote:
> 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. I didn't try with valgrind, indeed. However, I understand the problem now. :) > > 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. Okay. > > > 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. > I'm going to fix that. > > > > 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. :-) > I'll investigate on this part of code, don't worry. ;) > > 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. > Okay. > > 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 > > -- Best regards, Samuel Pitoiset.
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
