[FFmpeg-devel] [libav-devel] [PATCH] RTMP streaming crashes after some time with Flash Media Server (FMS), Issue 2236

[Manjunath Siddaiah]

-----Original Message-----
From: libav-devel-bounces at libav.org [mailto:libav-devel-bounces at libav.org] On Behalf Of Kostya
Sent: Thursday, March 24, 2011 3:50 AM
To: libav development
Subject: Re: [libav-devel] [PATCH] RTMP streaming crashes after some time with Flash Media Server (FMS), Issue 2236

On Wed, Mar 23, 2011 at 05:49:56PM +0000, Manjunath Siddaiah wrote:
> Hi,
> 
> Issue 2236 is reproducible for high bitrates more than 1 Mbps. Current RTMP implementation assumes there will be one "rtmp packet" inside a "flv packet".
> This assumption is true for low bitrates and for high bitrates there will be multiple "rtmp packets" inside a "flv packet". Current implementation has not taken care of this situation.
> The fix is done taking care off "multiple rtmp packets" in a "flv packet".  The patch has 3 line changes in "rtmp_write" function in "libavformat/rtmpproto.c" and is provided with suitable comments.
> 
> The patch is tested overnight for video bitrate at 10 Mbps without any crashing.
> 
> -Manjunath H Siddaiah
 
Not that I know a thing about RTMP but here's a quick review. 

> Index: libavformat/rtmpproto.c
> ===================================================================
> --- libavformat/rtmpproto.c	(revision 26402)
> +++ libavformat/rtmpproto.c	(working copy)
> @@ -930,8 +930,8 @@
>      int pktsize, pkttype;
>      uint32_t ts;
>      const uint8_t *buf_temp = buf;
> -
> -    if (size < 11) {
> +    // rt->flv_off will never be always zero if there are multiple 
> + rtmp packets inside one flv packet

That comment should be forbidden by Geneve convention as it's self-contradictory and causes harm to native English readers.

> +    if (!rt->flv_off && size < 11) {
>          av_log(LOG_CONTEXT, AV_LOG_DEBUG, "FLV packet too small %d\n", size);
>          return 0;
>      }

IIRC FLV packet header is 11 bytes and FLV header is 12-13 bytes, so I don't see why it should be ignored for input packets.

> @@ -972,8 +972,12 @@
>          if (rt->flv_size - rt->flv_off > size_temp) {
>              bytestream_get_buffer(&buf_temp, rt->flv_data + rt->flv_off, size_temp);
>              rt->flv_off += size_temp;
> +            // Making size_temp = 0 when it reads full of buffer
> +            size_temp = 0;
>          } else {
>              bytestream_get_buffer(&buf_temp, rt->flv_data + 
> rt->flv_off, rt->flv_size - rt->flv_off);
> +            // Reducing size_temp by as many bytes as read from the buffer
> +            size_temp -= (rt->flv_size - rt->flv_off);
>              rt->flv_off += rt->flv_size - rt->flv_off;
>          }

that also looks reasonable but would be better without comments _______________________________________________
libav-devel mailing list
libav-devel at libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Hi,

Re submitting the patch with comments removed.

-Manjunath H Siddaiah
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: PATCH_for_Issue_2236.txt
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110411/83c10f7c/attachment.txt>