GNOME Bugzilla – Bug 672701
mpegvideoparse: high cpu usage breaks playback of mpeg2 video in mkv on dreambox
Last modified: 2012-05-22 12:56:59 UTC
ftp://82.149.226.170/BrokenPlayback2.mkv gst-launch playbin2 used to be able to play this file just fine up to 0.10.35 now it's entirely broken, if i'm lucky i get a big blocked still frame every few seconds. playback commandline: GST_DEBUG_NO_COLOR=1 GST_DEBUG=*:5 gst-launch playbin2 uri=file:///media/hdd/BrokenPlayback2.mkv flags=1 >log_35.log 2>&1 logfiles and pipeline graphs to be attached
ftp://test4711:test4711@82.149.226.170/bug672701_resources.tar.bz2 for the debug logs and pipeline graphs (too large for bugzilla) test4711:test4711 is also the login for the test mkv
further experiments have shown that gst-launch filesrc location=BrokenPlayback2.mkv ! matroskademux ! dvbvideosink works just fine but when introducing the respective video parser element gst-launch filesrc location=BrokenPlayback2.mkv ! matroskademux ! mpegvideoparse ! dvbvideosink then we get the same exact error pattern like when using playbin2 the problem may be caused by excess cpu load without the parser, playback consumes 3-6%, with playbin2 it's at full load this problem is limited to our embedded architecture with a hardware decoder, on the pc it doesn't cause problems and it's apparently limited to MPEG2 streams, h264parse doesn't cause this kind of problems
Created attachment 213760 [details] [review] mpegvideoparser: Optimize scanning for start code
Created attachment 213761 [details] [review] mpegvideoparser: Fix gst_mpeg_video_parse to parse a single frame The approach is to stop processing the buffer when a complete frame has been parsed. This is one solution, another way around could be to mimic the mpeg4 parser and only parse 1 packet per _parse() call.
Source can also be seen at: http://cgit.collabora.com/git/user/nicolas/gst-plugins-bad.git/log/?h=672701-mpegvideoparse-slow
Review of attachment 213760 [details] [review]: Doesn't look wrong but do you also have some numbers (just curious :) )? And do the addition of new branches in the code really improve performance that much, especially as this doesn't look very friendly to branch prediction? Might make sense to use lookup table maybe? Couldn't you use the byte reader scanning function here instead (and optimize that the same way?)?
Review of attachment 213761 [details] [review]: Is this really always parsing everything up to a frame? Shouldn't it request more data from the base class if no data is left and you're not at the frames=2 break?
I ran callgrind on a HD mpeg2 video file and I get the following (averaged instructions/calls) instruction/calls | git | +1st patch | +2nd patch ------------------------------------------------------------------ gst_mpeg_video_parse | 1 446 974 | 420 373 | 310 743 (29.1%) | (21.5%) So essentially it's 5 times faster.
(In reply to comment #6) > Review of attachment 213760 [details] [review]: > > Doesn't look wrong but do you also have some numbers (just curious :) )? And do > the addition of new branches in the code really improve performance that much, > especially as this doesn't look very friendly to branch prediction? Might make > sense to use lookup table maybe? The checks are actually laid out in order of biggest probability, the branch prediction is therefore the closest to reality. For a perfectly random sequence, there's a 254/256 (99.2%) probability that the 3rd byte is > 1, followed by a 255/256 (99.6%) probabilty that the 2nd byte is non null. I'd say the prediction is pretty well established :) The impact on L1 read miss is on the same order, since you might need to "read back" (i.e. refetch) data which you don't have locally... but that only happens with a 1/128 probability.
The problem seems due to the codec parsing lib here. In the mpeg4 case the codec parser allows scanning from one start code to the next and is used as such by the parser element, which retains full state control that way (and similarly so for h264). In the mpeg2 case the codec parser wants to parse it all and return a list of start code position. One possible problem is that it scans more than needed. The patch tries to prevent this by tracking some state in the codec parsing lib scan, but that is only possible if the data is always provided (to the lib) from the frame's start (otherwise it will not manage to track state properly). However, that too is inefficient since that means always scanning from the start. So, IMO, the mpeg1/2 codec lib should preferably be adjusted more in line with the others.
> So, IMO, the mpeg1/2 codec lib should preferably be adjusted more in line with > the others. +1 (and perhaps we could get rid of the GList stuff too somehow?)
So done: [0.10] commit 100fddfbe913ad0fc87b13231473b7024f521337 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue May 22 14:00:36 2012 +0200 mpegvideoparse: tweak codec parser API and adjust parser element ... to allow for more efficient parsing and (more) consistent parsing API among various codec parsers. Fixes #672701.
[0.11] commit 28f3858b94c19929bf6eeb1ebb4442cdee0e46b2 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue May 22 14:00:36 2012 +0200 mpegvideoparse: tweak codec parser API and adjust parser element ... to allow for more efficient parsing and (more) consistent parsing API among various codec parsers. Fixes #672701. Conflicts: gst/videoparsers/gstmpegvideoparse.c