After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 735379 - mpeg2dec: use stride information from GstVideoFrame
mpeg2dec: use stride information from GstVideoFrame
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 743311 (view as bug list)
Depends on:
Blocks: 735360
 
 
Reported: 2014-08-25 13:00 UTC by Gwenole Beauchesne
Modified: 2015-03-13 17:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mpeg2dec: use stride information from GstVideoFrame (1.51 KB, patch)
2014-08-25 13:01 UTC, Gwenole Beauchesne
needs-work Details | Review
mpeg2dec: use stride information from GstVideoFrame (3.01 KB, patch)
2014-08-25 13:44 UTC, Gwenole Beauchesne
none Details | Review
[PATCH] mpeg2dec: Re-implement pool handling (20.42 KB, patch)
2015-03-10 18:11 UTC, Nicolas Dufresne (ndufresne)
none Details | Review

Description Gwenole Beauchesne 2014-08-25 13:00:40 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.
Comment 1 Gwenole Beauchesne 2014-08-25 13:01:32 UTC
Created attachment 284400 [details] [review]
mpeg2dec: use stride information from GstVideoFrame
Comment 2 Sebastian Dröge (slomo) 2014-08-25 13:13:13 UTC
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
Comment 3 Gwenole Beauchesne 2014-08-25 13:44:04 UTC
(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.
Comment 4 Gwenole Beauchesne 2014-08-25 13:44:27 UTC
Created attachment 284402 [details] [review]
mpeg2dec: use stride information from GstVideoFrame
Comment 5 Nicolas Dufresne (ndufresne) 2015-03-09 19:19:16 UTC
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 ?
Comment 6 Nicolas Dufresne (ndufresne) 2015-03-09 19:24:56 UTC
*** Bug 743311 has been marked as a duplicate of this bug. ***
Comment 7 Nicolas Dufresne (ndufresne) 2015-03-09 19:30:22 UTC
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.
Comment 8 Nicolas Dufresne (ndufresne) 2015-03-10 18:11:53 UTC
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(-)
Comment 9 Nicolas Dufresne (ndufresne) 2015-03-10 18:20:30 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2015-03-11 15:08:02 UTC
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?
Comment 11 Nicolas Dufresne (ndufresne) 2015-03-11 15:44:54 UTC
(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.
Comment 12 Nicolas Dufresne (ndufresne) 2015-03-13 17:03:09 UTC
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