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 638891 - tee: needs to aggregate QoS THROTTLE events from all source branches
tee: needs to aggregate QoS THROTTLE events from all source branches
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-01-07 08:54 UTC by Yongnian
Modified: 2018-11-03 12:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for gstevent.c (2.39 KB, patch)
2011-01-07 08:56 UTC, Yongnian
none Details | Review
patch for gstevent.h (2.06 KB, patch)
2011-01-07 08:57 UTC, Yongnian
none Details | Review
patch for gstquark.c (821 bytes, patch)
2011-01-07 08:58 UTC, Yongnian
none Details | Review
patch for gstquark.h (613 bytes, patch)
2011-01-07 08:58 UTC, Yongnian
none Details | Review
patch for gsttee.c (8.18 KB, patch)
2011-01-07 08:59 UTC, Yongnian
none Details | Review
patch for gstvideosink.c (4.88 KB, patch)
2011-01-07 09:00 UTC, Yongnian
none Details | Review
patch for new api in gstreamer core (5.33 KB, patch)
2011-01-10 12:18 UTC, Yongnian
none Details | Review
patch for event generation in gstvideosink in gst-plugins-base (4.48 KB, application/octet-stream)
2011-01-10 12:20 UTC, Yongnian
  Details
patch for event generation in gstvideosink in gst-plugins-base (4.48 KB, patch)
2011-01-10 12:22 UTC, Yongnian
none Details | Review
patch for event processing in gsttee in gstreamer core (7.10 KB, patch)
2011-01-10 12:23 UTC, Yongnian
none Details | Review
QOS event extension in gstreamer core (10.77 KB, patch)
2011-01-20 06:15 UTC, Yongnian
none Details | Review
Event processing of gsttee in gstreamer core (7.39 KB, patch)
2011-01-20 06:16 UTC, Yongnian
needs-work Details | Review
Event generation of gstvideosink of gst-plugins-base (5.43 KB, patch)
2011-01-20 06:17 UTC, Yongnian
rejected Details | Review

Description Yongnian 2011-01-07 08:54:04 UTC
There might be many cases that some window is invisible 
 (for example other window is in full screen), its related sink elements are still busy in rendering and upstream elements are busy in processing and decoding. Here a new event type "QOS_OBSCURED" is introduced. 

The UI toolkit (Gtk+, Qt) already keeps track of windows status information: visible or invisible etc. So it would be straightforward to extend the GstVideoSink class to have a property that encourages degraded rendering. When such property is set, the "QOS_OBSCURED" event is pushed to upstream elements for handling: degraded quality for less resources. 

And, for bonus points, as there is a trend in the industry toward stream switching for video over the internet, such event could be used to select appropriate media info in future.

After discussion with David Schleef (ds@entropywave.com) and Halley Zhao (halley.zhao@intel.com), the patches (based on version 0.10.28) for such mechanism are ready for video rendering degradation when it is invisible. They include below changes:
1. gstquark.h/gstquark.c: new quark string
2. gstevent.h/gstevent.c: new QoS event 
3. gstvideosink.c: generate new QoS event when it encourages degraded rendering
3. gsttee.c: degrade handling rate when it receives degraded rendering request
Comment 1 Yongnian 2011-01-07 08:56:26 UTC
Created attachment 177721 [details] [review]
patch for gstevent.c
Comment 2 Yongnian 2011-01-07 08:57:29 UTC
Created attachment 177722 [details] [review]
patch for gstevent.h
Comment 3 Yongnian 2011-01-07 08:58:09 UTC
Created attachment 177723 [details] [review]
patch for gstquark.c
Comment 4 Yongnian 2011-01-07 08:58:46 UTC
Created attachment 177724 [details] [review]
patch for gstquark.h
Comment 5 Yongnian 2011-01-07 08:59:33 UTC
Created attachment 177725 [details] [review]
patch for gsttee.c
Comment 6 Yongnian 2011-01-07 09:00:21 UTC
Created attachment 177726 [details] [review]
patch for gstvideosink.c
Comment 7 Wim Taymans 2011-01-07 09:34:34 UTC
QOS_OBSCURED sounds like a really bad name to implement stream selection IMO. I remember thaytan once had a more complete solution for doing this.
Comment 8 Tim-Philipp Müller 2011-01-07 10:18:52 UTC
Also, please submit patches in git format-patch format instead of one-diff-per-file (ie. then one patch to add the new API to core, one patch to use it in tee, one patch to add support to GstVideoSink)
Comment 9 Yongnian 2011-01-10 12:18:44 UTC
Created attachment 177923 [details] [review]
patch for new api in gstreamer core
Comment 10 Yongnian 2011-01-10 12:20:25 UTC
Created attachment 177924 [details]
patch for event generation in gstvideosink in gst-plugins-base
Comment 11 Yongnian 2011-01-10 12:22:32 UTC
Created attachment 177926 [details] [review]
patch for event generation in gstvideosink in gst-plugins-base
Comment 12 Yongnian 2011-01-10 12:23:33 UTC
Created attachment 177927 [details] [review]
patch for event processing in gsttee in gstreamer core
Comment 13 Yongnian 2011-01-10 12:28:25 UTC
A better event name than QOS_OBSCURED is appreciated! Also welcome more complete patch to merge together. BTW, I am still new to the git and please let me know if the patch doens't work or fit into your process.
Comment 14 Wim Taymans 2011-01-10 12:35:02 UTC
Why can't this be done with normal Qos messages? wouldn't it be better to totally disable the stream?
Comment 15 Wim Taymans 2011-01-10 12:39:02 UTC
Why would you want to add this complex code to tee? What does it solve, why is it needed?
Comment 16 Yongnian 2011-01-10 12:58:05 UTC
I discussed this with David Schleef
(http://sourceforge.net/mailarchive/forum.php?thread_name=20101213042030.GA20961%40cooker.entropywave.com&forum_name=gstreamer-devel)
at gstreamer developer forum. Two ways are possible: a specialized QoS event
(proportion=0.0) or a new QoS event. We think the second way is better: 1) many
codes might be updated for such special proportion for prior way; 2) the
semantic is different from original QoS event which is only impacted by CPU
overhead to disable stream; 3) it might be extended to new usage scenario to
select stream with different quality. As why we cannot completedly disable the
stream, sink element still needs "render" the invisible window for future
re-painting request (window system doesn't hold previously rendered data).
For the last question, the element of TEE is reducing the rate of buffer processing when it sees QOS_OBSCURED event, but it cannot push such event to upstream elements further for its funtionality.
Comment 17 Wim Taymans 2011-01-10 14:31:50 UTC
(In reply to comment #16)
> I discussed this with David Schleef
> (http://sourceforge.net/mailarchive/forum.php?thread_name=20101213042030.GA20961%40cooker.entropywave.com&forum_name=gstreamer-devel)
> at gstreamer developer forum. Two ways are possible: a specialized QoS event
> (proportion=0.0) or a new QoS event.

That's not what david suggested, he suggested an extension, not a new event. Why is the proportion=0.0 not sufficient for you?

> We think the second way is better: 1) many codes might be updated for such special proportion for prior way;

How would that work? what would they do? What would they do differently from QOS ?

> 2) the semantic is different from original QoS event which is only impacted by CPU overhead to disable stream;

That is true, maybe suggesting that current QOS is not the right approach. Or maybe suggesting that a QOS extension is needed.

> 3) it might be extended to new usage scenario to select stream with different quality.

That would be better served with a stream selection mechanism, IMO.

> As why we cannot completedly disable the
> stream, sink element still needs "render" the invisible window for future
> re-painting request (window system doesn't hold previously rendered data).

How would that work? what is the element supposed to do? How many frames per second is acceptable?

> For the last question, the element of TEE is reducing the rate of buffer
> processing when it sees QOS_OBSCURED event, but it cannot push such event to
> upstream elements further for its funtionality.

I thought the point was to reduce the amount of unneeded processing done by decoders. Is it that you only forward the event when all branches of tee received the event?


What I see here that you want to add an event to instruct upstream element to reduce the rate of data processing. This is exactly what QOS does. Maybe you simply want to add a flag to QOS to tell it that the rate reduction is not because of realtime performance problems?
Comment 18 Yongnian 2011-01-11 02:28:26 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > I discussed this with David Schleef
> > (http://sourceforge.net/mailarchive/forum.php?thread_name=20101213042030.GA20961%40cooker.entropywave.com&forum_name=gstreamer-devel)
> > at gstreamer developer forum. Two ways are possible: a specialized QoS event
> > (proportion=0.0) or a new QoS event.
> That's not what david suggested, he suggested an extension, not a new event.
> Why is the proportion=0.0 not sufficient for you?
> > We think the second way is better: 1) many codes might be updated for such special proportion for prior way;
> How would that work? what would they do? What would they do differently from
> QOS ?
> > 2) the semantic is different from original QoS event which is only impacted by CPU overhead to disable stream;
> That is true, maybe suggesting that current QOS is not the right approach. Or
> maybe suggesting that a QOS extension is needed.
> > 3) it might be extended to new usage scenario to select stream with different quality.
> That would be better served with a stream selection mechanism, IMO.
That is the future possible usage, if any developer needs such feature, they could extend current path for their purpose. It's hard for me to expect exactly what they need and implement what they like. 
> > As why we cannot completedly disable the
> > stream, sink element still needs "render" the invisible window for future
> > re-painting request (window system doesn't hold previously rendered data).
> How would that work? what is the element supposed to do? How many frames per
> second is acceptable?

That is currently handled in the element gstxvimagesink, the "rendered" data was saved for future repainting. If we completely disable stream, when "repainting" request happens, we can only repaint the window with buffer long long ago. Currently the frame per second for obscured sink window is set to 1 frame per second statically.

> > For the last question, the element of TEE is reducing the rate of buffer
> > processing when it sees QOS_OBSCURED event, but it cannot push such event to
> > upstream elements further for its funtionality.
> I thought the point was to reduce the amount of unneeded processing done by
> decoders. Is it that you only forward the event when all branches of tee
> received the event?

The QOS_OBSCURED event will be pushed to upstream elements. Not all elements could pass through such event. Those elements which would adjust the rate of buffer processing for such event should not push it to upstream further. So TEE is such element. Generally only one src branch of TEE is sink element and TEE need adjusts its buffer transmit rate to such src branch. But all other src branches and upstream should not be impacted. 
 
> What I see here that you want to add an event to instruct upstream element to
> reduce the rate of data processing. This is exactly what QOS does. Maybe you
> simply want to add a flag to QOS to tell it that the rate reduction is not
> because of realtime performance problems?

Could you please give more hints on what suggesting QoS event extension would be? To add a flag to indicate the exact QoS reason? If so, I'm afraid that many codes might to be updated since the semantics are different. If most of others can accept that other related codes don't need to be updated, that's fine for all.
if (event_type == QOS_EVENT) {
   //original processing
}
==>
if (event_type == QOS_EVENT) {
    if (qos_resson == CPU_OVERHEAD) {
        //original processing
    }
    else if (qos_reason == SINK_OBSCURED) {
        //new stuffs
    }
}
Comment 19 Yongnian 2011-01-20 06:15:20 UTC
Created attachment 178810 [details] [review]
QOS event extension in gstreamer core
Comment 20 Yongnian 2011-01-20 06:16:22 UTC
Created attachment 178811 [details] [review]
Event processing of gsttee in gstreamer core
Comment 21 Yongnian 2011-01-20 06:17:07 UTC
Created attachment 178812 [details] [review]
Event generation of gstvideosink of gst-plugins-base
Comment 22 Yongnian 2011-01-20 06:26:10 UTC
(In reply to comment #17)
According to your review feedback, I revised the modification as below:
1. avoid introduction another new QOS event. Add one field/parameter to indicate the reason why QOS event is generated. In order to backward compatibility, the new API has suffix as "_full" which is required to provide new paramter "reason". For the original API without "_full" is still kept as the wrapper to new full API. 
2. the future extension for stream source selection by stream quality is added as well as one of the QOS reason in QoS event. But the specific QOS event generation and processing are not implemented (leaving for the developers who need it in future)
3. Others kept the same, the complexcity in GSTTEE cannot be reduced, since it need handle whether one src path can use degraded rate of buffer for rendering while others can use normal rate of buffer for rendering. If you have better idea, please help raise it and it will be appreciated!

> (In reply to comment #16)
> > I discussed this with David Schleef
> > (http://sourceforge.net/mailarchive/forum.php?thread_name=20101213042030.GA20961%40cooker.entropywave.com&forum_name=gstreamer-devel)
> > at gstreamer developer forum. Two ways are possible: a specialized QoS event
> > (proportion=0.0) or a new QoS event.
> That's not what david suggested, he suggested an extension, not a new event.
> Why is the proportion=0.0 not sufficient for you?
> > We think the second way is better: 1) many codes might be updated for such special proportion for prior way;
> How would that work? what would they do? What would they do differently from
> QOS ?
> > 2) the semantic is different from original QoS event which is only impacted by CPU overhead to disable stream;
> That is true, maybe suggesting that current QOS is not the right approach. Or
> maybe suggesting that a QOS extension is needed.
> > 3) it might be extended to new usage scenario to select stream with different quality.
> That would be better served with a stream selection mechanism, IMO.
> > As why we cannot completedly disable the
> > stream, sink element still needs "render" the invisible window for future
> > re-painting request (window system doesn't hold previously rendered data).
> How would that work? what is the element supposed to do? How many frames per
> second is acceptable?
> > For the last question, the element of TEE is reducing the rate of buffer
> > processing when it sees QOS_OBSCURED event, but it cannot push such event to
> > upstream elements further for its funtionality.
> I thought the point was to reduce the amount of unneeded processing done by
> decoders. Is it that you only forward the event when all branches of tee
> received the event?
> What I see here that you want to add an event to instruct upstream element to
> reduce the rate of data processing. This is exactly what QOS does. Maybe you
> simply want to add a flag to QOS to tell it that the rate reduction is not
> because of realtime performance problems?
Comment 23 Wim Taymans 2011-02-09 19:11:20 UTC
Ok, After some research, I think I know how this should go forwards now.

First the different QOS types need to go into the QOS event. We need to be able to tell upstream if the sink is OVERFLOWed or UNDERFLOWed. These QOS types would normally be generated when the sink has to drop frames because they are late (underflow) in the sink or when the sink is consuming too much time processing the frames (overflow). We currently only use UNDERFLOW but when we measure the render time in the sink we could also generate OVERFLOW.

Then we also want to be able to control the amount of frames per second that sinks will process. We would call this framerate throttling. The videosink (or even basesink) would have a new property to control the throttle interval (or the desired time interval between buffers, which maps directly to a max framerate).  

A new QOS type, THROTTLE would be added and we would set the timestamp of the qos event to that of the last rendered buffer and the diff to a positive value corresponding to the configured throttle interval (by default 0). This thus simply adds the throttle interval to the next allowed synchronization time. 

The proportion would be set to the desired slowdown needed to get the desired framerate. All existing elements will be able to deal with this correctly and improved elements can use the new QOS type and proportion to tweak their implementation if needed.

It looks like these patches are pretty close to this, I'll try to merge them and update the design docs with this new use case.
Comment 24 Sebastian Dröge (slomo) 2011-02-09 19:35:59 UTC
FWIW this totally makes sense to me and sounds like a useful extension of the QoS events.
Comment 25 Yongnian 2011-02-10 06:00:25 UTC
Hi Wim Taymans, thank you! That sounds pretty perfect. If you need any other info during merge, don't hesitate to let me know and I would like to help.
Comment 26 Wim Taymans 2011-02-10 14:47:14 UTC
Committed the throttle-time property on basesink to core now. Now it's only that strange patch to tee that's left to investigate.
Comment 27 Yongnian 2011-02-11 01:09:15 UTC
Thanks you! Please let me try to explain. Hope below works and makes sense, and welcome any better solution or idea. 

The purpose of "tee" is try to duplicate source buffers and pass them down to multiple sinks. If all sinks of tee generate "throttle" QoS event, "tee" can push such event to upstream elemens, which might be aware of such event and take corresponding action to reduct the buffer rate consequently.

But at most time, if only one sink of "tee" generates "throttle" QoS event, it is not safe for "tee" to push such event to upstream elements. So "tee" needs take care itself: namely drop some of the buffers from source to that sink which generates "throttle" QoS event; while "tee" still needs pass down all buffers to other sinks which don't generate "throttle" QoS event. That's the work of the patch to "tee". 

In order to avoid the frequent variance caused by "throttle" QoS event, above "throttle" behavior in "tee" take some delay period before invalidation,
namely (3 * throttle_time_interval). If there is no new "throttle" QoS event from sink to renew/keep such behavior, the "tee" works as traditional mode.
Comment 28 Sebastian Dröge (slomo) 2011-04-01 08:51:50 UTC
The reason for this tee patch makes sense to me, we should get this in before the release IMHO. Otherwise tee could cause problems if this new QoS type is used. Wim?
Comment 29 Sebastian Dröge (slomo) 2011-04-17 17:20:59 UTC
This is marked as blocker for 0.10.33. Any reason to keep it as blocker?
Comment 30 Tim-Philipp Müller 2011-04-17 17:31:07 UTC
> This is marked as blocker for 0.10.33. Any reason to keep it as blocker?

Don't think so.
Comment 31 Sebastian Dröge (slomo) 2011-05-26 11:12:45 UTC
Comment on attachment 178812 [details] [review]
Event generation of gstvideosink of gst-plugins-base

I think this should be solved differently by implementing it in the subclasses, which might be able to actually check if the window is obscured or not.
Comment 32 Sebastian Dröge (slomo) 2011-05-26 11:15:05 UTC
Comment on attachment 178811 [details] [review]
Event processing of gsttee in gstreamer core

Please update the tee patch for the new QOS event type names and also add a more descriptive commit message :)
Comment 33 Edward Hervey 2013-08-13 18:44:50 UTC
This bug has an assigned developer but has not received activity in almost a year.

Is the assigned person still working on this ?
Comment 34 Tim-Philipp Müller 2016-11-11 21:37:26 UTC
We have a QoS THROTTLE type now and a throttle-time property on GstBaseSink.

Retitling to reflect what's left to do, namely making tee aggregate throttle QoS in some way.
Comment 35 GStreamer system administrator 2018-11-03 12:14:38 UTC
-- 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/gstreamer/issues/18.