On Wed, 5 Sep 2007 13:27:31 +0200
"Benjamin Otte" <otte at gnome.org> wrote:
> Hi,
>
> I'm one of the Swfdec[1] Flash player developers. As you may be aware,
> Flash uses VP6 as a possible video decoder.
> Someone recently grabbed all of the videos at http://pown.alluc.org
> and threw them at ffmpeg. Surprisingly, lots of them killed the vp6
> decoder and in turn my browser.
>
> I'll attached a series of patches for issues that I could fix myself,
> and point out issues I could not fix myself.
> If you want to reproduce the issues, you'll have to get swfdec git,
> wget the files I'll point to and try to play them with player/swfplay
> from the swfdec sources.
I want to reproduce the issue, but I want to do it with ffmpeg/ffplay
(instead of swfdec). As the swf demuxer don't support compressed swf,
I tried to convert the samples using swf2flv (from FLV::Info). It
failed to convert some of the samples, so I wasn't able to test
everything.
(BTW: any information about compressed swf format would be appreciated,
is it simply a zlib compressed swf or something like that ?)
> -- SEGV
> http://pown.alluc.org/179.swf
> patch: check_coeff_offset.diff
> SInce this was the first file where I hit the issue, I'll just list
> this one. FFmpeg reads an offset and doesn't validate it, this patch
> does that.
This sample failed to convert so I wasn't able to test, but the fix
seems OK. I will probably apply something similar soon.
(BTW, already said in previous review, but please don't use tabs in
your patches)
> -- sanity checks
> patch: sanity.diff
> While trying to find the issue, I added sanity checks to the read
> functions used by vp56, as there doesn't seem to be any protection
> against overreads.
I don't like it very much. Moreover it probably has a significant
impact on performances. So at least a benchmark would be needed
(+ taking care of all remarks in other reviews).
> -- small memleak
> patch: small_memleak.diff
> This is a small memleak fix I noticed when looking at the code.
This looks OK (except the unneeded if(ctx)).
But I'm not the maintainer of imgresample.c so I will let approval
to our beloved maintainer ;-)
> -- memleak
> http://pown.alluc.org/210.swf
> http://pown.alluc.org/991.swf
> There's a huge memleak reported by valgrind (this is from a run of
210.swf):
> ==31127== 224,784,144 bytes in 177 blocks are still reachable in loss
> record 5,098 of 5,098
> ==31127== at 0x4021990: memalign (vg_replace_malloc.c:332)
> ==31127== by 0x510ECE6: av_malloc (mem.c:61)
> ==31127== by 0x4D71B01: avcodec_default_get_buffer (utils.c:308)
> ==31127== by 0x4F4F789: vp56_decode_frame (vp56.c:509)
> ==31127== by 0x4D70B7D: avcodec_decode_video (utils.c:937)
> ==31127== by 0x406712D: swfdec_video_decoder_ffmpeg_decode
> (swfdec_codec_ffmpeg.c:235)
> ==31127== by 0x406A2A8: swfdec_video_decoder_decode
> (swfdec_codec_video.c:102)
> ==31127== by 0x40A32F0: swfdec_video_input_iterate (swfdec_video.c:82)
> ==31127== by 0x40A40F8: swfdec_video_movie_iterate_end
> (swfdec_video_movie.c:92)
> ==31127== by 0x4089287: swfdec_player_iterate (swfdec_player.c:1114)
> ==31127== by 0x4089533: swfdec_player_do_advance (swfdec_player.c:1156)
> ==31127== by 0x407A53A: swfdec_marshal_VOID__ULONG_UINT
> (swfdec_marshal.c:246)
Unable to reproduce. No ffmpeg related memleak here when playing 991.flv
with ffplay (I wasn't able to convert 210.swf to flv).
So I assume this memleak is due to bad usage of lavc by swfdec.
> -- "alternative entropy decoding not supported"
> http://pown.alluc.org/179.swf
> http://pown.alluc.org/185.swf
> http://pown.alluc.org/207.swf
> http://pown.alluc.org/243.swf
> http://pown.alluc.org/376.swf
> http://pown.alluc.org/453.swf
> http://pown.alluc.org/498.swf
> http://pown.alluc.org/652.swf
> http://pown.alluc.org/653.swf
> http://pown.alluc.org/658.swf
> http://pown.alluc.org/661.swf
> http://pown.alluc.org/673.swf
> http://pown.alluc.org/756.swf
> http://pown.alluc.org/845.swf
> http://pown.alluc.org/850.swf
> http://pown.alluc.org/860.swf
> http://pown.alluc.org/962.swf
> http://pown.alluc.org/966.swf
> http://pown.alluc.org/990.swf
> http://pown.alluc.org/1040.swf
> http://pown.alluc.org/1063.swf
> http://pown.alluc.org/1064.swf
> http://pown.alluc.org/1106.swf
> http://pown.alluc.org/1126.swf
> The VP6 decoder often claims the above error message and then produces
> weird images. So feel free to use the files listed here if you want to
> implement it. I should also notice that these files do a lot of other
> weird stuff (like using invalid image sizes for the decoded image). I
> guess that's just a side effect?
I was able to convert more than half of them to flv, and none of them
shows the "alternative entropy decoding not supported" message...
> -- SEGV in mmx code
> http://pown.alluc.org/280.swf
> http://pown.alluc.org/416.swf
> http://pown.alluc.org/497.swf
> http://pown.alluc.org/830.swf
> There's a SEGV here. I have no clue where it comes from. I'll leave
> that for you to figure out:
> #0 0xb6f69cfa in put_pixels8_mmx (block=0xb6a827a8
"????????ds\t",
> pixels=0xb611d6a8 '?' <repeats 200 times>..., line_size=1464,
h=8)
> at i386/dsputil_mmx.c:416
> #1 0xb71431d6 in vp56_decode_frame (avctx=0x80d6c30, data=0x80d24a0,
> data_size=0xbfa4b008,
> buf=0x8114980
> "..."...,
> buf_size=2840) at vp56.c:432
> #2 0xb6f63b7e in avcodec_decode_video (avctx=0x80d6c30,
> picture=0x80d24a0, got_picture_ptr=0xbfa4b008,
> buf=0x8114980
> "..."...,
> buf_size=2840) at utils.c:937
No crash here, but I guess the mmx version is not used on my computer.
I will try to force it's usage later, to try reproducing it.
Aurel