GNOME Bugzilla – Bug 743311
mpeg2dec does not cope if downstream buffer allocation has stride != width
Last modified: 2015-03-09 19:24:56 UTC
mpeg2dec assumes that stride == width. This causes corrupt video with: gst-launch-1.0 filesrc location=file.ts ! tsdemux ! mpegvideoparse ! mpeg2dec ! vaapisink (or any other sink where stride != width) I've got a patch I'll attach to fix this, which also asserts that the stride is something libmpeg2 can cope with.
Created attachment 295127 [details] [review] [PATCH] mpeg2dec: Fix handling of stride A pipeline like: gst-launch-1.0 filesrc location=file.ts ! tsdemux ! mpegvideoparse ! mpeg2dec ! vaapisink would look bad when file.ts contains 704x576 video, because vaapisink would give you buffers of stride 768, but libmpeg2 was not told about this and used a stride of 704. Tell libmpeg2 about the stride from downstream; in the process, add asserts documenting libmpeg2's current stride assumptions. Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> --- ext/mpeg2dec/gstmpeg2dec.c | 6 ++++++ 1 file changed, 6 insertions(+)
Review of attachment 295127 [details] [review]: ::: ext/mpeg2dec/gstmpeg2dec.c @@ +559,3 @@ + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 1) == chroma_stride); + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 2) == chroma_stride); + mpeg2_stride (mpeg2dec->decoder, GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 0)); A buffer could be allocated during decide_allocation(), this way you can check this and reject the pool if not met ?
(In reply to comment #2) > Review of attachment 295127 [details] [review]: > > ::: ext/mpeg2dec/gstmpeg2dec.c > @@ +559,3 @@ > + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 1) == chroma_stride); > + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 2) == chroma_stride); > + mpeg2_stride (mpeg2dec->decoder, GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 0)); > > A buffer could be allocated during decide_allocation(), this way you can check > this and reject the pool if not met ? I'm not clear on how I'd do that safely. Can I assume that the pool is active during decide_allocation? If not, is it safe for me to active and deactivate it? Can I use the GstVideoDecoder allocation functions to get a buffer and release it at this point, or do I not have enough state set up yet (what *is* "enough state" in this context)? If I can't use the GstVideoDecoder allocation functions, where do I get my GstVideoInfo from?
(In reply to comment #3) > (In reply to comment #2) > > Review of attachment 295127 [details] [review] [details]: > > > > ::: ext/mpeg2dec/gstmpeg2dec.c > > @@ +559,3 @@ > > + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 1) == chroma_stride); > > + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 2) == chroma_stride); > > + mpeg2_stride (mpeg2dec->decoder, GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 0)); > > > > A buffer could be allocated during decide_allocation(), this way you can check > > this and reject the pool if not met ? > > I'm not clear on how I'd do that safely. > > Can I assume that the pool is active during decide_allocation? If not, is it > safe for me to active and deactivate it? The element that implement the decide_allocation is also the one the will enable the pool. It is safe to activate the pool in decide_allocation, activating a second time later in the base class will simply return success. Don't forget to deactivate it if you reject the pool. > > Can I use the GstVideoDecoder allocation functions to get a buffer and release > it at this point, or do I not have enough state set up yet (what *is* "enough > state" in this context)? You could, but ideally you would make that test before chaining the decide_allocation(). > > If I can't use the GstVideoDecoder allocation functions, where do I get my > GstVideoInfo from? You don't really need that. From the allocated buffer, you will extract the GstVideoMeta. This structure contains a stride array. If there is no GstVideoMeta, you can assume that the stride will match your expectation.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Review of attachment 295127 [details] [review] [details] [details]: > > > > > > ::: ext/mpeg2dec/gstmpeg2dec.c > > > @@ +559,3 @@ > > > + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 1) == chroma_stride); > > > + g_assert (GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 2) == chroma_stride); > > > + mpeg2_stride (mpeg2dec->decoder, GST_VIDEO_FRAME_PLANE_STRIDE (&vframe, 0)); > > > > > > A buffer could be allocated during decide_allocation(), this way you can check > > > this and reject the pool if not met ? > > > > I'm not clear on how I'd do that safely. > > > > Can I assume that the pool is active during decide_allocation? If not, is it > > safe for me to active and deactivate it? > > The element that implement the decide_allocation is also the one the will > enable the pool. It is safe to activate the pool in decide_allocation, > activating a second time later in the base class will simply return success. > Don't forget to deactivate it if you reject the pool. > > > > > Can I use the GstVideoDecoder allocation functions to get a buffer and release > > it at this point, or do I not have enough state set up yet (what *is* "enough > > state" in this context)? > > You could, but ideally you would make that test before chaining the > decide_allocation(). > > > > > If I can't use the GstVideoDecoder allocation functions, where do I get my > > GstVideoInfo from? > > You don't really need that. From the allocated buffer, you will extract the > GstVideoMeta. This structure contains a stride array. If there is no > GstVideoMeta, you can assume that the stride will match your expectation. The documentation for GstVideoMeta says that the stride array may not always be valid. Is this a case where it's guaranteed to be valid?
(In reply to comment #5) > The documentation for GstVideoMeta says that the stride array may not always be > valid. Is this a case where it's guaranteed to be valid? In real life it's always valid. But if you want to be by the book (I would probably remove that from the documentation imho), you can get a GstVideoInfo with gst_video_info_from_caps(). Then you can use this VideoInfo to map the buffer using gst_video_frame_map(). The VideoInfo will be updated and the stride array at this point has to be valid.
Created attachment 295201 [details] [review] [PATCH] mpeg2dec: Fix handling of stride A pipeline like: gst-launch-1.0 filesrc location=file.ts ! tsdemux ! mpegvideoparse ! mpeg2dec ! vaapisink would look bad when file.ts contains 704x576 video, because vaapisink would give you buffers of stride 768, but libmpeg2 was not told about this and used a stride of 704. Tell libmpeg2 about the stride from downstream; in the process, teach it to reject buffer pools that don't meet libmpeg2's chroma stride requirements Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> --- ext/mpeg2dec/gstmpeg2dec.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)
This version has been tested, and works for me - I don't have a sink that has funky strides to test, though.
Review of attachment 295201 [details] [review]: ::: ext/mpeg2dec/gstmpeg2dec.c @@ +352,3 @@ gst_video_codec_state_unref (state); + return chroma_stride_acceptable; Nearly there. Failing here would mean failing the pipeline. To reject a pool, what one usually do is to update the allocation with same size/min/max but pool set to NULL. And finally return true. The base class will crate a generic video pool for you.
Created attachment 295211 [details] [review] [PATCH] mpeg2dec: Fix handling of stride A pipeline like: gst-launch-1.0 filesrc location=file.ts ! tsdemux ! mpegvideoparse ! mpeg2dec ! vaapisink would look bad when file.ts contains 704x576 video, because vaapisink would give you buffers of stride 768, but libmpeg2 was not told about this and used a stride of 704. Tell libmpeg2 about the stride from downstream; in the process, teach it to reject buffer pools that don't meet libmpeg2's chroma stride requirements Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> --- ext/mpeg2dec/gstmpeg2dec.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
Have I finally understood enough of what I'm doing? Or am I still not getting it?
Comment on attachment 295127 [details] [review] [PATCH] mpeg2dec: Fix handling of stride My bad, sorry
Comment on attachment 295127 [details] [review] [PATCH] mpeg2dec: Fix handling of stride arg...
Ok, I'm sorry, it is more complicated then this. The pool need to be rejected before chaining to the parent. That also means you need to configure the pool, then activate it. I never tried this, but maybe there a way around with chaining a second time if needed. This way you don't have to configure the pool. I'm sorry I don't have a very precise answer here.
(In reply to comment #14) > Ok, I'm sorry, it is more complicated then this. The pool need to be rejected > before chaining to the parent. That also means you need to configure the pool, > then activate it. I never tried this, but maybe there a way around with > chaining a second time if needed. This way you don't have to configure the > pool. I'm sorry I don't have a very precise answer here. As far as I can tell from the code, I *have* to chain to the parent to get the pool with stride in the first place. What's the most sensible route from here? Unobsolete the original patch with asserts, and ask an expert to dive in?
Ah, I think the solution is quite simple, change the NULL into gst_video_buffer_pool_new ().
Review of attachment 295211 [details] [review]: ::: ext/mpeg2dec/gstmpeg2dec.c @@ +350,3 @@ +pool_not_activated: + if (!chroma_stride_acceptable) + gst_query_set_nth_allocation_pool (query, 0, NULL, size, min, max); Here, gst_video_buffer_pool_new() instead of NULL, and I think that's it.
Ah wait, this pool won't be configured if you do that.
Review of attachment 295211 [details] [review]: ::: ext/mpeg2dec/gstmpeg2dec.c @@ +327,3 @@ + goto buffer_not_acquired; + + if (!gst_video_frame_map (&vframe, &dec->decoded_info, test_buffer, I was testing this solution with some more changes and found that decoded_info is not always right. Because of the cropping that happens sometimes, the size in decoded_info might be bigger then the size the pool is configured.
*** This bug has been marked as a duplicate of bug 735379 ***