GNOME Bugzilla – Bug 795173
kms: kmsbufferpool won't return false, when the input size won't apply to it
Last modified: 2018-11-03 14:21:20 UTC
The kmsbufferpool will the use its priv->vinfo to alloc a buffer. The priv->vinfo comes from the caps in the gst_kms_buffer_pool_set_config(), parsed from the input bufferpool config. The size parameters in bufferpool config won't parsed, even it did in the set_config(), it won't apply to the bufferpool as well. As I said, the kmsbufferpool would use its' priv->vinfo to make a buffer, while the size filed from that GstVideoInfo is ignored. The kmsbufferpool would use the width, height and video pixel format information to calculate the reuqest size to the kernel's drm interface, then using the feedback size from the kernel to update the size filed in its priv->vinfo. When I apply a size parameter for the kmsbufferpool, it won't take effect at all, so the changing configure of the kmsbufferpool is not success at this time. It should returen an negative result(return FALSE).
Created attachment 370897 [details] [review] Only support allocate a larger buffer than default All the current hardware decoders of the Rockchip only has a register for the output address of the decoded image, so the luma plane and chroma plane must be stored in the same buffer.
The above patch won't work with the rockchip's 10bit pixel format. That format would store two pixel, for example, at the buffer beginning, the first full pixel and 6 bits of the next pixel would be stored in the beginning two bytes. When use that format, the stride means how many bytes should be used for a vertical pixel line. Then the stride from calculated from GstCaps would be much larger than the actually size.
Review of attachment 370897 [details] [review]: - We don't use Signed-off-by in GStreamer commits - I'm surprised this patch works for you, my impression is that the ksmsink code need to adapt to a larger buffer being allocated here. ::: sys/kms/gstkmsallocator.c @@ +188,3 @@ + + h = gst_drm_height_from_drm (fmt, GST_VIDEO_INFO_HEIGHT (vinfo)); + height = vinfo->size / (arg.bpp / 8) / arg.width; This is not valid for all the formats + this code should in the helper gst_drm_height_from_drm(), it was factored out to keep this part readable, now it's not. @@ +214,3 @@ + offs = GST_VIDEO_INFO_PLANE_OFFSET (vinfo, i); + else + GST_VIDEO_INFO_PLANE_OFFSET (vinfo, i) = offs; This is hard to understand.
(In reply to Randy Li (ayaka) from comment #2) > The above patch won't work with the rockchip's 10bit pixel format. > That format would store two pixel, for example, at the buffer beginning, the > first full pixel and 6 bits of the next pixel would be stored in the > beginning two bytes. > When use that format, the stride means how many bytes should be used for a > vertical pixel line. Then the stride from calculated from GstCaps would be > much larger than the actually size. I guess you mean that "height = vinfo->size / (arg.bpp / 8) / arg.width" fails, it will fail for all the formats currently marked GST_VIDEO_FORMAT_FLAG_COMPLEX.
I use a way to solve this problem with 10bit color pixel format, height = vinfo->size / arg.width; in the allocation_caps, when the pixel format is 10bit color pixel format gst_video_info_set_format (align_info, format, (hor_stride / 10) << 3, ver_stride); It would use the stride(pitch) calculation in kernel and bpp would expend its size. But I really don't want cheat the downstream element in this way.
https://bugzilla.gnome.org/show_bug.cgi?id=796516 It move the buffer allocation into an early stage, but it still can't overcome that there is no way check whether the size is accepted.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/688.