Created attachment 281008 [details] [review]
reorder the members of mpeg4_dmv_size_vlc_table[]

Do you have a sample stream that shows the problem?

Yes, the file provided in this bug report shows the problem (around frame 345 iirc, 10 seconds after the beginning) :
https://bugs.freedesktop.org/show_bug.cgi?id=71812#c11

The video there looks corrupted with avplay/ffmpeg too, and your patch does not change anything. Can you point at the relevant section of the spec for these changes so we can confirm it's a bug and not just a broken file?

In my case, the file plays fine with ffmpeg  but fails with a pipeline containing vaapidecode from gstreamer-vaapi. The difficulty to pinpoint this bug is that _only_ the vaapidecode element invokes gst_mpeg4_parse_video_object_plane(), which is the function that triggers this error inside parse_sprite_trajectory() . Using a pipeline with the mpeg4videoparse element only  wont invoke this function, so the problem will stay hidden in this case. 

The error is related to the way VLC (variable length code) data is parsed for the sprite trajectory for frame #345 of this sample file iirc. 

I spent some time wondering why the parsing of the _same_ bitfield was correct with ffmpeg, and returned buggy values with gst, finally failing with a bad marker bit detected by gst.

The bit stream should contain first the number of bits to be read using the VLC algorithm , secondly a value coded on the  number of bits computed earlier, and finally a marker bit that should be one. 

I discovered that the implementation of the VLC algorithm was different between ffmpeg (get_vlc2()) and gst-plugins-bad (decode_vlc()). I had the feeling that the ffmpeg implementation was correct, and the implementation of gst had problem, because starting at the same location in the bitstream, gst version fails when finding a wrong marker bit (0 instead of 1). 

So clearly the number of bits returned by decode_vlc() was wrong, the following value in the bitstream was read with a bad offset and a bad length, and the marker bit was checked at the wrong place.

Then I tried to understand what decode_vlc() was supposed to do. ffmpeg implementation in get_vlc2(), even if correct, was difficult to read due to heavy optimizations for speed. Until a found this older commit of the same function, that explains the role of each value of the VLC table a bit better : http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=7f4cf50496ef6797aaba819d0924bb5c4baf09a4

So, in the current implementation of decode_vlc(), in the VLC table for mpeg4_dmv_size_vlc_table , you should have values in this order :
+  {0, 0x00, 2},
+  {1, 0x02, 3},
+  {2, 0x03, 3},
+  {3, 0x04, 3},
+  {4, 0x05, 3}

the definition of the table being :
struct _VLCTable
{
  guint value;
  guint cword;
  guint cbits;
};

The algorithm of decode_vlc() being :
. for each triplet of the VLCTable:
  if the "cbits" first bits of the bitstream (3rd row) match the
  "cword" value (2nd row), then the decoded value to be returned should
  be "value" value (1st row), else continue.

(the equivalent table in ffmpeg is ff_sprite_trajectory_tab[15][2] in libavcodec/mpeg4data.h)

Makes sense, thanks. I missed that mpeg4videoparse does not even go into that code path.

But are you saying that you don't get any broken frames around 10s in that video if you use ffmpeg?

I'll push it right after 1.4.0 release btw

commit 2f28fcb86298ee19f604ac6f82802d35e71ee3d6
Author: Fabrice Bellet <fabrice@bellet.info>
Date:   Thu Jul 17 16:25:54 2014 +0200

    parser: mpeg4: fix vlc table used for sprite trajectory
    
    The vlc table members cbits, cword and values were assigned in the wrong
    order, causing the mpeg4 parser to fail when handling sprite
    trajectories.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733322

(In reply to comment #6)
> Makes sense, thanks. I missed that mpeg4videoparse does not even go into that
> code path.
> 
> But are you saying that you don't get any broken frames around 10s in that
> video if you use ffmpeg?

It depends of which decoder is used. With ffmpeg from fedora 20 (ffmpeg-2.1.5-1.fc20.x86_64) and its software mpeg4 decoder, the video plays fine. But using the vdpau decoder causes corruptions starting around 10s. I think they may be related to s-frames not being handled by the hardware decoder. I added some details about this in the freedesktop bug where I got the sample video.