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 763441 - rfbsrc: leads to -> invalid video buffer received
rfbsrc: leads to -> invalid video buffer received
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.x
Other All
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-03-10 09:47 UTC by iam.asm89
Modified: 2016-04-06 01:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Full debug log for the command described in the bug report. (363.26 KB, application/x-gzip)
2016-03-10 09:47 UTC, iam.asm89
  Details
rfbsrc: Properly handle negotiation, buffer pool setup and allocation of buffers (10.00 KB, patch)
2016-03-10 10:38 UTC, Sebastian Dröge (slomo)
needs-work Details | Review
rfbsrc: Unlock cancellable to make shutdown of the source instantaneously (2.46 KB, patch)
2016-03-10 10:38 UTC, Sebastian Dröge (slomo)
needs-work Details | Review
rfbsrc: Implement decide_allocation virtual (7.19 KB, patch)
2016-04-06 01:22 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description iam.asm89 2016-03-10 09:47:52 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.
Comment 1 iam.asm89 2016-03-10 09:49:48 UTC
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).
Comment 2 Sebastian Dröge (slomo) 2016-03-10 10:38:02 UTC
Created attachment 323605 [details] [review]
rfbsrc: Properly handle negotiation, buffer pool setup and allocation of buffers
Comment 3 Sebastian Dröge (slomo) 2016-03-10 10:38:07 UTC
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
Comment 4 Sebastian Dröge (slomo) 2016-03-10 10:40:49 UTC
(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.
Comment 5 Nicolas Dufresne (ndufresne) 2016-03-10 13:45:18 UTC
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.
Comment 6 Sebastian Dröge (slomo) 2016-03-10 18:01:12 UTC
(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.
Comment 7 Nicolas Dufresne (ndufresne) 2016-03-10 18:31:37 UTC
(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.
Comment 8 Nicolas Dufresne (ndufresne) 2016-03-15 00:21:42 UTC
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.
Comment 9 Nicolas Dufresne (ndufresne) 2016-04-05 18:03:04 UTC
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.
Comment 10 Nicolas Dufresne (ndufresne) 2016-04-05 20:01:29 UTC
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.
Comment 11 Nicolas Dufresne (ndufresne) 2016-04-06 01:22:05 UTC
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.
Comment 12 Nicolas Dufresne (ndufresne) 2016-04-06 01:22:50 UTC
Attachment 325457 [details] pushed as 7e293f1 - rfbsrc: Implement decide_allocation virtual