GNOME Bugzilla – Bug 656155
ffdec_mpeg2video and interlace property
Last modified: 2011-10-03 09:18:42 UTC
Created attachment 193416 [details] gst-launch log when playing an interlaced stream I think this plugin might be doing something fishy with the interlaced stream property (caps). I see the property not detected for some DVB-T/DVB-S streams while it should be, or even oscillating between interlaced and not-interlaced during play. Attached is a gst-launch log playing an interlaced stream. It seems to be detected at some points, but then lost in the chain? Am I seeing it right?
Some investigation shows that the ffmpeg decoder wrapper isn't changing the caps it attaches to buffers when ffmpeg changes, only the caps it uses for negotiation. I will look into this more tomorrow.
Looked a bit further, and not sure how best to fix this: In gstffmpegdec.c, we consistently gst_buffer_set_caps to the caps from ffmpegdec->srcpad. However, we also preallocate buffers with the old caps, before decoding a frame. The frame then goes out with the old caps, and isn't updated to the new caps. We need to find a suitable point at which to reset caps on the buffer if we've preallocated a buffer, but the caps have changed since.
Created attachment 194236 [details] [review] An ugly patch, which works around the interlacing issue, but which probably introduces new issues I don't think this patch is correct, beyond the obvious "compiles and works on a test stream" correctness. However, it results in the bug going away - things are correctly flagged as interlaced as they go through.
when the interlaced flag changes, the decoder renegotiates only after the frame is decoded. Looking at the code, it would seem best to include a check for that flag changing in gst_ffmpegdec_negotiate, so that renegotiation happens before the buffer is allocated. This should cause the new buffers to be created with the right caps from the start. I'll start doing that, but changes seem to be needed at quite a few different places, and I don't have any test case to ensure I don't forget any, or that this approach works at all. Would you happen to have a captured stream showing this happening ? Thanks
Created attachment 195076 [details] [review] Reset caps when interlaced flag changes It seems ffmpeg does not set the interlaced flag on the picture before calling the buffer alloc, so we cannot use that to renegotiate in time. So here's another patch similar to the one posted by Simon, which resets caps only when the interlaced flag has changed. Also only sets metadata writeable. Of related interest also is 6444bd2557139b80b6b9ca4e0b4856fd996b3ba1.
Could you please define "related interest" a bit more precisely?
Sorry - that commit's message says: "making the buffer metadata writable where it was done before pushing will always end up with a copy and that makes the sink do a slow memcpy all the time." which is an issue I wanted to point out, since the proposed patch does just that, making it worthwhile to switch writability only when necessary.
So you suggest taking 6444bd2557139b80b6b9ca4e0b4856fd996b3ba1 as well to avoid always doing slow copy?
You should have it already, no ? It would be odd if you don't, as you presumably wouldn't have had the interlaced caps issue in the first place, from what I can see, since the caps setting was (more or less) where Simon added it. Or am I misunderstanding you ?...
That patch is indeed already in. What were you trying to point out then, that the proposed patch from this bug report will make things slow? Which is exactly what I am seeing now when testing it. It is unusably slow.
OK, to recap: While looking at the patch Simon sent, I first thought I could get the buffer to have the right caps from the start. It turns out ffmpeg has not set the right flags yet when it calls the buffer allocation. So next, I thought we only need to reset caps when the interlaced flag changes. I did this. I looked at the history, and saw this patch that moved the caps setting from where it was (which is also where Simon moved it). I read the commit message, which mentioned the extra copy when making the buffer writable, so I pointed it out to ensure you were aware of the performance implication of making every buffer writable. Now, which patch are you using which makes things unusably slow ? Simon's original one, or my later one ?
Your patch plus the one for QOS messages. However using ffdec_mpeg2 is not a top priority - it is a nice to have because of it's multi-threadedness but issues relating to demuxing transport streams and issues in the core are more important.
Weird. My patch ought to only trigger when there is a change in interlacing. Maybe there's a subtle interaction with something else. I didn't notice any obvious change in performance, so could you share the sample where you see the slowdown ?
So I had a look at that to work out what I'm missing here. All the samples I have seem to not toggle between interlaced and not, could you please share one of those so I can test with it ? Cheers
Watching bbc-news.ts with gst-launch in verbose mode you don't see a constant spew of messages while playing? Did you try replicating in our environment with the packages I've uploaded?
Not on my laptop (with git gstreamer) nor on the onelan hardware as as, I did not try the rpm packages on your FTP though. Will the "reset to factory defaults" procedure from the installation guide allow me to revert to the default gst install if I overwrite the gst plugins with these packages now ?
Created attachment 196954 [details] [review] ffmpeg: ensure buffers have correct interlacedness in caps Whether a frame is interlaced or not is unknown at the time of buffer allocation, so caps on the buffer in opaque will have a previous frame's interlaced flag set. So if interlacedness changes, we update the buffer (if any) caps with the correct interlaced flag once we know.
Right, got it. More than the next buffer needs correcting after we detect an interlacing change. GStreamer was doing automatic caps updates invisibly when that wasn't done, which was slow. My machine hadn't quite reached the point where it was failing to keep up, so I did not notice the hit, but it was visible with top. Fixed by the latest attached patch, as an alternative choice to Simon's.
Looks fine, thanks!
commit d4d5e350d073bab89faf6a781ba5ef9df45f19b8 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Aug 29 15:18:39 2011 +0100 ffdec: ensure buffers have correct interlacedness in caps Whether a frame is interlaced or not is unknown at the time of buffer allocation, so caps on the buffer in opaque will have a previous frame's interlaced flag set. So if interlacedness changes, we update the buffer (if any) caps with the correct interlaced flag once we know. https://bugzilla.gnome.org/show_bug.cgi?id=656155