better bug-report

ORC_CODE=emulate \
GST_DEBUG="*:3,omxvideodec:5,omxh264dec:5,omxbufferpool:5" \
gst-launch-1.0  uridecodebin uri="http://live.mdragon.org/test_omx/test.ts" use-buffering=true name=demux  \
 ! tee name="decdata" \
 ! videoconvert \
 ! pngenc \
 ! multifilesink location="%03d.png" sync=true \
 2>&1

and one of the output file
http://live.mdragon.org/test_omx/001.png

I think you forgot to attach the logs. Would it be possible to run the same pipeline with the -v option, so we see the formats being negotiated. Also, adding videoconvert traces could help. This looks like a stride issue, we would need a trace to validate that.

(oh, and please make sure this isn't fixed in latest 1.2.4 stable release)

On my side I made a quick test on 1.3.1 on RPi with the following pipeline using the ts file provided:
gst-launch-1.0 filesrc location=~/tmp/test.ts ! tsdemux ! mpegvideoparse ! omxmpeg2videodec ! filesink location= /mnt/nfs/out/out.yuv

And the results is bad. So I think videoconvert is not involved
For information, the negotiated caps between omxvideodec and filesink are:
width: 720
height: 576
format: I420

By the way, I test with some other H264 or MPEG2 files which have the same definition and their output are also bad.
Like Nicolas, I think it's a stride issue. If I have some time, I will check stride handling in omxvideodec.

Thanks for the great feed back. Let us know you finding. If videoconvert does not do anything, I would definitely check the stride. Anyone knows the HW required alignment ? This would mean something larger then 16 bytes.

RPi HW decoder output the video frame with a stride of 736. So it seems that RPi adds padding to align on 32 bytes.

Great, very good start! Next step would be to check that GstVideoMeta is added to the buffers, and that the stride in it matches.

http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxbufferpool.c#n285

should be

pool->add_videometa =
        gst_buffer_pool_config_has_option (config,
        GST_BUFFER_POOL_OPTION_VIDEO_META) == 0;

Created attachment 278956 [details] [review]
debug init in omxbufferpool

Created attachment 278957 [details] [review]
add videometa properly

Review of attachment 278957 [details] [review]:

::: omx/gstomxbufferpool.c
@@ +290,3 @@
     pool->add_videometa =
         gst_buffer_pool_config_has_option (config,
+        GST_BUFFER_POOL_OPTION_VIDEO_META) == 0;

No, I doubt this is correct. We should add video meta if the option is enabled (not the opposite). Check where the pool is being configured, and make sure we do enable this option. Also, in this case we strictly need vieometa, if downstream don't support that meta we should fail cleanly. Same thing for encoder, if upstream wants to take the pool, but did not enable the meta, we should fail somehow.

My previous test did not work because filesink doesn't support video meta.
On 1.3.1, videoscale and videoconvert handle video meta unless they are set in passthrough mode.

By the way, Nicolas is right, we should only add video meta if the option is enabled in the pool.

(In reply to comment #11)
> Review of attachment 278957 [details] [review]:
> 
> ::: omx/gstomxbufferpool.c
> @@ +290,3 @@
>      pool->add_videometa =
>          gst_buffer_pool_config_has_option (config,
> +        GST_BUFFER_POOL_OPTION_VIDEO_META) == 0;
> 
> No, I doubt this is correct. We should add video meta if the option is enabled
> (not the opposite).
I agree with that.

> Also, in this case we strictly need vieometa, if
> downstream don't support that meta we should fail cleanly. Same thing for
> encoder, if upstream wants to take the pool, but did not enable the meta, we
> should fail somehow.
If we fail, we won't be able to use filesink or other elements which don't handle video stride. Maybe, we can rather do something in the decoder, or we can handle the stride meta in videoscale/videoconvert/... if downstream element doesn't support even if no other conversion is needed.

How can I check if downstream element support videometa?

videoscale/videoconvert support videometa

(In reply to comment #12)
> > Also, in this case we strictly need vieometa, if
> > downstream don't support that meta we should fail cleanly. Same thing for
> > encoder, if upstream wants to take the pool, but did not enable the meta, we
> > should fail somehow.
> If we fail, we won't be able to use filesink or other elements which don't
> handle video stride. Maybe, we can rather do something in the decoder, or we
> can handle the stride meta in videoscale/videoconvert/... if downstream element
> doesn't support even if no other conversion is needed.

Sorry, my response was clearly incomplete. GstVideoFrame and gst_video_frame_copy() will allow to copy from one stride to another. Just create a frame with "normal" stride, and copy to it as a slow-path fallback. We shouldn't care much about performance, writing raw to filesink is only useful for testing.

(In reply to comment #13)
> How can I check if downstream element support videometa?
> 
> videoscale/videoconvert support videometa

If you need example, I have implemented this in -good/sys/v4l2.

has_video_meta =
      gst_query_find_allocation_meta (query, GST_VIDEO_META_API_TYPE, NULL);

(In reply to comment #6)
> RPi HW decoder output the video frame with a stride of 736. So it seems that
> RPi adds padding to align on 32 bytes.

The code in gst_omx_buffer_pool_alloc_buffer() should take care of that in theory. However we need a fallback case here if downstream does not support the video meta. Which in your case all elements do? Would be good to see why omxh264dec does not see that

But as Nicolas said, you can easily implement the fallback case with gst_video_frame_copy()... or just reuse gst_omx_video_dec_fill_buffer() which is in the decoder and encoder already.

Can you check if that makes it work for you?

Ah, the tee element makes the videometa go away in your case.

Yes, the problem is the tee. It works without but not with tee... because tee currently can't aggregate the ALLOCATION queries and as such just drops it.

Which then means that omxvideodec can't use the videometa... but currently it does not convert the stride to the default GStreamer stride.

commit 4593f434a06d8d3b46338e0762e93c558ea4e58d
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Jun 24 14:52:58 2014 +0200

    omxbufferpool: Copy buffers if the stride does not match and we can't use video meta
    
    https://bugzilla.gnome.org/show_bug.cgi?id=731672

If I add videometa(with my patch) to omxbuferrpool it works fine with tee and without frame copy.
Are you sure that tee don't pass videometa?

See bug #730758

Your patch adds videometa although the ALLOCATION query says that it must not be used because of the above mentioned bug. You're just lucky that downstream of tee actually supports the videometa :)

But after your patch it really do frame copy,
even if videoconvert support videometa

Yes, but only if there is a tee involved. Due to the tee the decoders gets told that it can't use videometa, no matter what other elements are there. See bug #730758

Once that bug is fixed there will be no copying unless necessary.