Created attachment 177721 [details] [review]
patch for gstevent.c

Created attachment 177722 [details] [review]
patch for gstevent.h

Created attachment 177723 [details] [review]
patch for gstquark.c

Created attachment 177724 [details] [review]
patch for gstquark.h

Created attachment 177725 [details] [review]
patch for gsttee.c

Created attachment 177726 [details] [review]
patch for gstvideosink.c

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.

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)

Created attachment 177923 [details] [review]
patch for new api in gstreamer core

Created attachment 177924 [details]
patch for event generation in gstvideosink in gst-plugins-base

Created attachment 177926 [details] [review]
patch for event generation in gstvideosink in gst-plugins-base

Created attachment 177927 [details] [review]
patch for event processing in gsttee in gstreamer core

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.

Why can't this be done with normal Qos messages? wouldn't it be better to totally disable the stream?

Why would you want to add this complex code to tee? What does it solve, why is it needed?

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.

(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?

(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
    }
}

Created attachment 178810 [details] [review]
QOS event extension in gstreamer core

Created attachment 178811 [details] [review]
Event processing of gsttee in gstreamer core

Created attachment 178812 [details] [review]
Event generation of gstvideosink of gst-plugins-base

(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?

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.

FWIW this totally makes sense to me and sounds like a useful extension of the QoS events.

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.

Committed the throttle-time property on basesink to core now. Now it's only that strange patch to tee that's left to investigate.

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.

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?

This is marked as blocker for 0.10.33. Any reason to keep it as blocker?

> This is marked as blocker for 0.10.33. Any reason to keep it as blocker?

Don't think so.

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 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 :)

This bug has an assigned developer but has not received activity in almost a year.

Is the assigned person still working on this ?

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.

-- 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.