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 793008 - buffer: add GST_BUFFER_FLAG_NON_DROPPABLE
buffer: add GST_BUFFER_FLAG_NON_DROPPABLE
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 685279 792696
 
 
Reported: 2018-01-29 15:43 UTC by Mathieu Duponchelle
Modified: 2018-01-31 12:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstbuffer: add GST_BUFFER_FLAG_IMPORTANT (1.53 KB, patch)
2018-01-29 15:43 UTC, Mathieu Duponchelle
none Details | Review
gstbuffer: add GST_BUFFER_FLAG_NON_DROPPABLE (7.59 KB, patch)
2018-01-30 13:21 UTC, Mathieu Duponchelle
none Details | Review
gstbuffer: add GST_BUFFER_FLAG_NON_DROPPABLE (7.70 KB, patch)
2018-01-30 15:44 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2018-01-29 15:43:24 UTC
This can be used to identify buffers that should not be dropped
when reducing bandwidth, or for which a higher percentage of
redundancy should be allocated when performing forward error
correction.
Comment 1 Mathieu Duponchelle 2018-01-29 15:43:29 UTC
Created attachment 367581 [details] [review]
gstbuffer: add GST_BUFFER_FLAG_IMPORTANT
Comment 2 Nicolas Dufresne (ndufresne) 2018-01-29 15:45:46 UTC
This is pretty vague. Do you have some upstream code going to use that, and if so how is that going to be used ?
Comment 3 Mathieu Duponchelle 2018-01-29 15:54:47 UTC
(In reply to Nicolas Dufresne (stormer) from comment #2)
> This is pretty vague. Do you have some upstream code going to use that, and
> if so how is that going to be used ?

Yes, please see the blocked bug (https://bugzilla.gnome.org/show_bug.cgi?id=792696)
Comment 4 Nicolas Dufresne (ndufresne) 2018-01-29 16:10:05 UTC
It's still quite vague, why is this being generalized instead of just adding an RTP/FEC specific flag (or a name that is more descriptive) ? I'd like to understand how this flag is generally meaningful, how it should be interpreted, what's the boundary for it's usage.
Comment 5 Tim-Philipp Müller 2018-01-29 16:11:19 UTC
This is also needed for another use case (dvd still frames), see bug #685279.

Now the question is, what to call it! (I apologise).

FLAG_IMPORTANT

FLAG_NOT_DROPPABLE

FLAG_DO_NOT_DROP

I think I have a slight preference towards something like the last two because it's clearer what the meaning/implication is, and there's also a nice symmetry with FLAG_DROPPABLE then.
Comment 6 Tim-Philipp Müller 2018-01-29 16:12:50 UTC
> It's still quite vague, why is this being generalized instead of just adding
> an RTP/FEC specific flag (or a name that is more descriptive) ? I'd like to
> understand how this flag is generally meaningful, how it should be
> interpreted, what's the boundary for it's usage.

Olivier suggested this should be a general flag in core instead of RTP specific flags, and re-use the existing DROPPABLE flag. And it's useful for another case as well, see previous comment.
Comment 7 Nicolas Dufresne (ndufresne) 2018-01-29 16:23:27 UTC
I missed that dicussion. I like Tim's proposal to maintain a symmetry with the DROPPABLE flag, it's more precise. Is UNDROPPABLE a word ? Would GST_BUFFER_FLAG_KEEP be an option, or maybe I'm missing the sense ? I really think that "IMPORTANT" is not quite right.
Comment 8 Mathieu Duponchelle 2018-01-29 22:32:59 UTC
As usual, naming stuff is hard :) I would go for NON_DROPPABLE to keep the symmetry, though "undroppable" exists on wiktionary it doesn't ring nice to my ears, apologies for the bike shedding but I didn't start it :P
Comment 9 Tim-Philipp Müller 2018-01-29 22:39:59 UTC
I think we almost have a winner.

Jan - which one sounds best to you? (NOT_DROPPABLE, NON_DROPPABLE, UNDROPPABLE)
Comment 10 Jan Schmidt 2018-01-30 02:27:13 UTC
NON_DROPPABLE gets my vote too
Comment 11 Mathieu Duponchelle 2018-01-30 13:21:18 UTC
Created attachment 367644 [details] [review]
gstbuffer: add GST_BUFFER_FLAG_NON_DROPPABLE

This can be used to identify buffers that should not be dropped
when reducing bandwidth, or for which a higher percentage of
redundancy should be allocated when performing forward error
correction.
Comment 12 Tim-Philipp Müller 2018-01-30 13:42:24 UTC
Comment on attachment 367644 [details] [review]
gstbuffer: add GST_BUFFER_FLAG_NON_DROPPABLE

>This can be used to identify buffers that should not be dropped
>when reducing bandwidth, or for which a higher percentage of
>redundancy should be allocated when performing forward error
>correction.

I don't really understand the reference to 'when reducing bandwidth'. The phrase comes from the DROPPABLE flag description I guess, I just don't know if it needs to be kept around.

>diff --git a/common b/common
>index 3fa2c9e37..3f4aa969c 160000
>--- a/common
>+++ b/common
>@@ -1 +1 @@
>-Subproject commit 3fa2c9e372bceec30be91e67fb02b6cb05bed493
>+Subproject commit 3f4aa969cbe39584a649d98d4cf321d78bd73092


common submodule commit should be removed.


>+ * @GST_BUFFER_FLAG_NON_DROPPABLE: the buffer can not be dropped without breaking the
>+ *         stream, for example to reduce bandwidth.


I would remove the "for example to reduce bandwidth" bit here.

How about "This buffer is important and should not be dropped. This can be used to mark important buffers, e.g. to flag RTP packets carrying keyframes or codec setup data for RTP Forward Error Correction purposes, or to prevent still video frames from being dropped by elements due to QoS."

Or is this too much detail?

Also let's add a "(Since: 1.14)" at the end of the description line.

Lastly, should we move up the @NON_DROPPABLE in the docs chunk to be next to the DROPPABLE one?

In any case, I think this is good to go in.
Comment 13 Mathieu Duponchelle 2018-01-30 15:44:45 UTC
Created attachment 367649 [details] [review]
gstbuffer: add GST_BUFFER_FLAG_NON_DROPPABLE

This can be used to identify buffers for which a higher percentage
of redundancy should be allocated when performing forward error
correction, or to prevent still video frames from being dropped by
elements due to QoS.
Comment 14 Mathieu Duponchelle 2018-01-30 15:47:44 UTC
(In reply to Tim-Philipp Müller from comment #12)
> I don't really understand the reference to 'when reducing bandwidth'. The
> phrase comes from the DROPPABLE flag description I guess, I just don't know
> if it needs to be kept around.

Yep, that was just copy pasted

> common submodule commit should be removed.

my bad

> Or is this too much detail?

I don't think it's too much detail, giving examples is always useful

> Lastly, should we move up the @NON_DROPPABLE in the docs chunk to be next to
> the DROPPABLE one?

I usually keep the same order for the fields and their documentation, don't really have a preference, just tell me if you do prefer doing it that way :)
Comment 15 Mathieu Duponchelle 2018-01-31 12:34:54 UTC
Attachment 367649 [details] pushed as ebcdfd2 - gstbuffer: add GST_BUFFER_FLAG_NON_DROPPABLE