GNOME Bugzilla – Bug 763441
rfbsrc: leads to -> invalid video buffer received
Last modified: 2016-04-06 01:23:32 UTC
Created attachment 323599 [details] Full debug log for the command described in the bug report. When using rfbsource connected to the VNC server exposed by an android emulator the pipeline will not produce output at the sink and gst-launch will complain: $ gst-launch-1.0 rfbsrc host=localhost port=49948 view-only=true ! videoconvert ! x264enc ! mpegtsmux ! hlssink Setting pipeline to PAUSED ... Pipeline is live and does not need PREROLL ... Redistribute latency... Setting pipeline to PLAYING ... New clock: GstSystemClock ** (gst-launch-1.0:17997): CRITICAL **: gst_video_frame_map_id: assertion 'info->finfo->format == meta->format' failed WARNING: from element /GstPipeline:pipeline0/GstVideoConvert:videoconvert0: Internal GStreamer error: code not implemented. Please file a bug at http://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer. Additional debug info: gstvideofilter.c(292): gst_video_filter_transform (): /GstPipeline:pipeline0/GstVideoConvert:videoconvert0: invalid video buffer received ^Chandling interrupt. Interrupt: Stopping pipeline ... Execution ended after 0:00:00.611242893 Setting pipeline to PAUSED ... I've attached the full debug log.
I used to following command to start an android emulator with a VNC server exposed on port 49948: $ emulator64-x86 -no-boot-anim -prop ro.test_harness=true -partition-size 1024 -scale 1.0 -skin WVGA800 -wipe-data -avd foobar -ports 36739,44743 -no-window -debug-all -noaudio -qemu -m 512 -enable-kvm -vnc :44048 The command assumes you have created an avd named "foobar" (e.g. android avd create -n foobar -t 20).
Created attachment 323605 [details] [review] rfbsrc: Properly handle negotiation, buffer pool setup and allocation of buffers
Created attachment 323606 [details] [review] rfbsrc: Unlock cancellable to make shutdown of the source instantaneously Doesn't work well though, it runs into a loop of ** (gst-launch-1.0:15860): CRITICAL **: unknown message type 200
(In reply to Sebastian Dröge (slomo) from comment #2) > Created attachment 323605 [details] [review] [review] > rfbsrc: Properly handle negotiation, buffer pool setup and allocation of > buffers The colors seem off with this one, at least for RGB16.
Review of attachment 323605 [details] [review]: ::: gst/librfb/gstrfbsrc.c @@ +370,3 @@ gst_buffer_pool_config_add_option (config, GST_BUFFER_POOL_OPTION_VIDEO_META); + return gst_buffer_pool_set_config (pool, config); Check you return value. This may return FALSE if the configuration had to be changed. There is a helper for the general case, see gst_buffer_pool_config_validate_params(). Also, enabling VideoMeta need to depend on downstream having video meta.
(In reply to Nicolas Dufresne (stormer) from comment #5) > Review of attachment 323605 [details] [review] [review]: > > ::: gst/librfb/gstrfbsrc.c > @@ +370,3 @@ > gst_buffer_pool_config_add_option (config, > GST_BUFFER_POOL_OPTION_VIDEO_META); > > + return gst_buffer_pool_set_config (pool, config); > > Check you return value. This may return FALSE if the configuration had to be > changed. If the configuration had to be changed, what else than failing can we do? :) > Also, enabling VideoMeta need to depend on downstream having video meta. Yes, this was wrong before though, rfbsrc does not even need videometa or has any advantage of it. And there's a lot similarly broken things in there. :) This is for now only a WIP, someone with enough time can spend some more on it. The current code is far from great.
(In reply to Sebastian Dröge (slomo) from comment #6) > (In reply to Nicolas Dufresne (stormer) from comment #5) > > Review of attachment 323605 [details] [review] [review] [review]: > > > > ::: gst/librfb/gstrfbsrc.c > > @@ +370,3 @@ > > gst_buffer_pool_config_add_option (config, > > GST_BUFFER_POOL_OPTION_VIDEO_META); > > > > + return gst_buffer_pool_set_config (pool, config); > > > > Check you return value. This may return FALSE if the configuration had to be > > changed. > > If the configuration had to be changed, what else than failing can we do? :) Some changes are acceptable, but need to be communicated (e.g. having infinit max buffer changed to finit maximum number of buffers). I did port all decide_allocation I could find. The only exception is when you configure you own pool, or when we know we are configuring the generic video pool. In that case, you don't expect it to change anything.
Review of attachment 323606 [details] [review]: Well if you do that, rfbdecoder will endup in unknown state. When you cancel, you also need to get rid of the connection. Note, we should check the return value of the _read/send calls.
Review of attachment 323606 [details] [review]: I have implemente similar thing in 52b50d. I don't reset the cancellable, as the connection is not recoverable if you stop in the middle of an operation. Instead of doing random stuff (and often stall), it will simply fail if you flush without restarting the element.
OK, as it is now, we cannot use downstream pool, as we ignore the video meta (stride). Though, the way it works, is that we need to copy assembled frame to a downstream allocated frame. We could use GstVideoFrame API for the copy, and that would handle the stride properly.
Created attachment 325457 [details] [review] rfbsrc: Implement decide_allocation virtual This way we can use the base class for buffer allocation, hence use fill() instead of create() virtual. This also adds a strict check on the select pool buffer size as we don't support strides and padding. This is based on initial patch proposed by Sebastien Dröge, from which I also fixed a buffer pool leak.
Attachment 325457 [details] pushed as 7e293f1 - rfbsrc: Implement decide_allocation virtual