GNOME Bugzilla – Bug 735379
mpeg2dec: use stride information from GstVideoFrame
Last modified: 2015-03-13 17:03:21 UTC
mpeg2dec wrongly assumes that buffers are allocated with a stride that is a multiple of 4 pixels. This assumption predates GstVideoMeta information whereby explicit strides could be provided from downstream allocated buffers.
Created attachment 284400 [details] [review] mpeg2dec: use stride information from GstVideoFrame
Comment on attachment 284400 [details] [review] mpeg2dec: use stride information from GstVideoFrame Best to add a check for the other strides and error out (or copy!) if not matching
(In reply to comment #2) > (From update of attachment 284400 [details] [review]) > Best to add a check for the other strides and error out (or copy!) if not > matching Well, fallbacking to copy would imply some more changes to further factor out with the video cropping code. :) So, I have currently error'ed out in the new patch. I would simply consider the downstream allocator in error if it provides inconsistent/abnormal stride information.
Created attachment 284402 [details] [review] mpeg2dec: use stride information from GstVideoFrame
Review of attachment 284402 [details] [review]: ::: ext/mpeg2dec/gstmpeg2dec.c @@ +557,3 @@ + for (i = 1; i < GST_VIDEO_FRAME_N_PLANES (&vframe); i++) { + const guint pstride = GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, i); + if (pstride != (stride >> mpeg2dec->chroma_w_shift)) Maybe use already existing video info scale macro ?
*** Bug 743311 has been marked as a duplicate of this bug. ***
I think all the approaches so far are failing because they don't address the problem too well. From more reading, mpeg2dec requires allocation to be 16bytes aligned and this seems to be covered. But then it also require a stride alignment of 16, and a pixel height alignment of 16. Finally this patch address the issue that it required uniformed strides. If we can cover all this, that would be much better. The decide allocation method is also very off. This decoder is very similar to libav video decoders, except that it does not offer a method to copy crop. Fortunately, it's always a top left align crop, which we can handle with video_frame_copy.
Created attachment 299041 [details] [review] [PATCH] mpeg2dec: Re-implement pool handling This is a rewrite of the pool negotiation and configuration. Direct to output decoding is now achieved by configuring the pool using video-alignment. This removes copies when dealing with any elements that supports VideoAlignment, and enable usage of generic video buffer pool, XVImagePool and GLPool. It drops the crop meta implementation for now. https://bugzilla.gnome.org/show_bug.cgi?id=735379 --- ext/mpeg2dec/gstmpeg2dec.c | 384 ++++++++++++++++++++++++--------------------- ext/mpeg2dec/gstmpeg2dec.h | 5 +- 2 files changed, 207 insertions(+), 182 deletions(-)
So I'm posting this patch at a first step. For the next step we need to do two things. As we can only provide 1 stride for all the planes, we need to check how mpeg2dec scales the stride, and make sure the pool provide buffers with strides that are compatible. I think the easiest is to activate the pool, and acquire a buffer. Then get the VideoMeta and check. I think it's faire to assume this won't change for buffers of the same pool. Then we just have to tell mpeg2dec as found in previous patches. This check is only needed if we need_alignment of course.
You should still check if it doesn't change for future buffers, and error out if it does. Also the stride might change after a reconfiguration (e.g. because now we got a different pool from downstream). Why did you remove the crop support?
(In reply to Sebastian Dröge (slomo) from comment #10) > You should still check if it doesn't change for future buffers, and error > out if it does. Also the stride might change after a reconfiguration (e.g. > because now we got a different pool from downstream). I still think a pool shouldn't change it's alignment dynamically. Of course, in the following patch I'll have robust code. I'm not done yet as you can see, as I just re-oriented the code toward a design that I know is well covered, and tested. > > Why did you remove the crop support? The implemented crashed, and was wrong in many ways. There is nothing to reuse there really. I'll re-add later, though I'd focus on theora. Unlike video-alignment method, CROP meta is only rarely supported. Nearly all the transform element drop that from the query unless passthrough, even videocrop does not yet implement it. When we get the in theoradec (since there is no other solution there, unless the crop is chroma aligned). I'd be happy to re-add this feature later. Meanwhile, video-alignment is a much more powerful optimization, and I don't believe we should care to much about CROP here.
commit 35c937f2a45e23762bbfb43ce3378d4d05f48d25 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Mar 13 17:00:03 2015 +0000 mpeg2dec: Add stride support This allow using external pools that have different strides from the default. These strides need to respect certain rules, which we check and if these are not met, we fallback to generic pool. https://bugzilla.gnome.org/show_bug.cgi?id=735379 commit 7e8050728a26b02317d85afcc9456b1e6630bf32 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Mar 10 16:45:01 2015 +0000 mpeg2dec: Re-implement pool handling This is a rewrite of the pool negotiation and configuration. Direct to output decoding is now achieved by configuring the pool using video-alignment. This removes copies when dealing with any elements that supports VideoAlignment, and enable usage of generic video buffer pool, XVImagePool and GLPool. It drops the crop meta implementation for now. https://bugzilla.gnome.org/show_bug.cgi?id=735379