From: Thomas V. S. <th...@ur...> - 2002-08-29 09:19:36
|
Hi, I've been tearing my hair out the last two days because I had some really weird issues with a program I was writing. I finally tracked down what the problem was and realized I didn't really know what the best way was to fix it in gstreamer-land. Here's the thing : * I get audio data from an input pipe * this data goes into a tee which feeds two analysis pipes * the analysis pipe each use the same filter (bpwsinc) to filter data, then go through cutter and to fakesink To be efficient, I coded the bpwsinc filter to do an in-place edit on the buffers. Of course, tee sends out pointers to the buffer, and thus both filters work on the same data in-memory. That's the root of my problem. Now, two obvious ways to quickly fix it a) have tee send out copies of the buffers instead of pointers to the same buffer (and maybe add a copy flag to tee so you can request this behaviour) b) have bpwsinc not do an in-place edit. Now, the problem here is a pretty general one, so, this should probably be solved on a higher level. I know there are provisions in GStreamer to mark a buffer as read-only. So, here are the questions : a) should each plug-in that wants to do in-place editing for efficiency check if it's allowed to by himself and make a copy if necessary ? b) if so, when does a buffer get marked read-only, and who should be doing this ? (ie, I could imagine tee making all of it's output buffers read-only by default if it is sending out more than one set of them) c) should this change to tee be made ? Any thoughts appreciated. I might be overlooking something that's already there ;) Thomas -- The Dave/Dina Project : future TV today ! - http://davedina.apestaart.org/ <-*- -*-> Hold me in your arms I want to be your only possession <-*- th...@ap... -*-> URGent, the best radio on the Internet - 24/7 ! - http://urgent.rug.ac.be/ |
From: Cameron H. <cam...@xd...> - 2002-08-29 12:55:25
|
Once upon a time Thomas Vander Stichele said... > [...] > > To be efficient, I coded the bpwsinc filter to do an in-place edit on the > buffers. > Of course, tee sends out pointers to the buffer, and thus both filters > work on the same data in-memory. That's the root of my problem. > > [...] > > So, here are the questions : > > a) should each plug-in that wants to do in-place editing for efficiency > check if it's allowed to by himself and make a copy if necessary ? This would seem to be correct. I'm only just getting into gstreamer so I've been reading the manual, and chapter 10 of the application development manual says: A more complex case is when the filter modifies the data in place. It does so and simply passes on the buffer to the next element. This is just as easy to deal with. An element that works in place has to be careful when the buffer is used in more than one element; a copy on write has to made in this situation. Since buffers are reference counted, you could use that to see if you are the only one holding a reference (I think). If you're going to do an inplace modification, you could then copy the buffer and unreference the old one. I'm only going by what I've read. I haven't done any of this stuff (yet). |
From: Thomas V. S. <th...@ur...> - 2002-08-29 15:00:08
|
Hi, > > a) should each plug-in that wants to do in-place editing for efficiency > > check if it's allowed to by himself and make a copy if necessary ? > > This would seem to be correct. I'm only just getting into gstreamer so > I've been reading the manual, and chapter 10 of the application > development manual says: Heh, it's great that you comment on the manual ! I've been so wrapped up in internals that I rarely bother to check it anymore, I should do that more often. Thanks for replying, and let us know if you find anything in there that's not clear or right so we can work on it. > A more complex case is when the filter modifies the data in place. It > does so and simply passes on the buffer to the next element. This is > just as easy to deal with. An element that works in place has to be > careful when the buffer is used in more than one element; a copy on > write has to made in this situation. > > Since buffers are reference counted, you could use that to see if you > are the only one holding a reference (I think). Yeah, theoretically, but this could get nasty very quickly. I don't know if it's safe to use the reference count for this, and you'd probably have to do locking for it to work. I've thought about it some more, and like to make this process as transparent as possible. At this moment, I made some changes to tee, and I changed my plug-in to make a copy of the buffer when the buffer is read-only. IMO, this sort of code should actually go in the code that passes buffers between elements; ie. gst_pad_push and gst_pad_pull. I would let an element declare if it does in-place editing through a property or flag or whatever, and the code for the buffer passing could then check if the buffer is read-only and the element receiving the buffer does in-place-edits. In that case, it should make a copy and unref the original one. Would that work ? The other thing that's needed then is the change to tee I made, which I think is reasonable; if tee sends out the same buffer to more than one pad, all of those buffers should be read-only. Thomas -- The Dave/Dina Project : future TV today ! - http://davedina.apestaart.org/ <-*- -*-> You came in just like smoke With a little come on come on come on in your walk come on <-*- th...@ap... -*-> URGent, the best radio on the Internet - 24/7 ! - http://urgent.rug.ac.be/ |
From: Richard B. <ri...@ta...> - 2002-08-29 15:48:34
|
On Thu, 2002-08-29 at 15:59, Thomas Vander Stichele wrote: > > Since buffers are reference counted, you could use that to see if you > > are the only one holding a reference (I think). > > Yeah, theoretically, but this could get nasty very quickly. I don't know > if it's safe to use the reference count for this, and you'd probably have > to do locking for it to work. How about making a helper function which takes a buffer and returns a buffer which is safe to modify. This function would work by getting appropriate locks, checking the buffer reference count, returning the same buffer if the refcount is 1 (so noone else is looking at the buffer), or taking a copy of the buffer if the refcount is 0. Then, before an element modifies a buffer, it should _always_ call the helper function to get a buffer that's safe to modify. I'm not sure that having a special flag to set is as useful as having a function to call: perhaps the element only occasionally wants to modify a buffer, but normally passes through, for example. (For example, an element which does a fade out at the beginning and end of a song, but passes buffers through unchanged otherwise) The problem with a tee simply marking all its buffers readonly is that you then force all the buffers to be copied, instead of allowing the final one to be used without copying it. In other words, you force an extra buffer copy. I'm not sure how important this is, but it would be nice to avoid it. -- Richard |
From: Thomas V. S. <th...@ur...> - 2002-08-29 16:30:12
|
> On Thu, 2002-08-29 at 15:59, Thomas Vander Stichele wrote: > > > Since buffers are reference counted, you could use that to see if you > > > are the only one holding a reference (I think). > > > > Yeah, theoretically, but this could get nasty very quickly. I don't know > > if it's safe to use the reference count for this, and you'd probably have > > to do locking for it to work. > > How about making a helper function which takes a buffer and returns a > buffer which is safe to modify. This function would work by getting > appropriate locks, checking the buffer reference count, returning the > same buffer if the refcount is 1 (so noone else is looking at the > buffer), or taking a copy of the buffer if the refcount is 0. Maybe the lifetime of a buffer should be more clearly documented. For example, is it guaranteed that if the refcount is 1, no other part is still able to get a ref to it ? ie, are there situations where some other part of the system could ref the buffer, expecting it to be the same as it was originally, while some plug-in modified it in-place ? > Then, before an element modifies a buffer, it should _always_ call the > helper function to get a buffer that's safe to modify. > I'm not sure that having a special flag to set is as useful as having a > function to call: perhaps the element only occasionally wants to modify > a buffer, but normally passes through, for example. (For example, an > element which does a fade out at the beginning and end of a song, but > passes buffers through unchanged otherwise) Hm, good points. The overhead of either a flag or this extra function is about the same. So, should I be adding this function ? What should it be called and where should it go ? > The problem with a tee simply marking all its buffers readonly is that > you then force all the buffers to be copied, instead of allowing the > final one to be used without copying it. In other words, you force an > extra buffer copy. I'm not sure how important this is, but it would be > nice to avoid it. Yeah. It makes the code harder though. For now I just want to add some bits that a) solve my problem at this point, so I can run my app based on the next gstreamer release b) are extendable to do the right thing in the future, without having to change the API. I'd like to do this quickly and before the next release, so I can update all the filters I've written to take this into account, so they're actually usable ;) Thomas -- The Dave/Dina Project : future TV today ! - http://davedina.apestaart.org/ <-*- -*-> I love the way you love but I hate the way I'm supposed to love you back <-*- th...@ap... -*-> URGent, the best radio on the Internet - 24/7 ! - http://urgent.rug.ac.be/ |
From: Richard B. <ri...@ta...> - 2002-08-29 21:31:28
|
On Thu, 2002-08-29 at 17:27, Thomas Vander Stichele wrote: > Maybe the lifetime of a buffer should be more clearly documented. For > example, is it guaranteed that if the refcount is 1, no other part is > still able to get a ref to it ? ie, are there situations where some other > part of the system could ref the buffer, expecting it to be the same as it > was originally, while some plug-in modified it in-place ? Surely, if the refcount is 1, there should be no other references to the buffer. I havn't looked into the refcount system used here, but it would be very surprising if a refcount of 1 didn't imply that no other piece of code has a reference to it. Of course, it's always possible that your code will take a copy and thus increase the refcount. But that's under your control. So, I'd say "no, there shouldn't be situations where some other part of the system could ref the buffer if the refcout is 1". Someone more familiar with the code feel free to correct, and surprise, me. > So, should I be adding this function ? What should it be called and where > should it go ? I think so. "gst_buffer_make_writable()" or something. It should probably be able to take a readonly buffer and return a writable copy, too. > > The problem with a tee simply marking all its buffers readonly is that > > you then force all the buffers to be copied, instead of allowing the > > final one to be used without copying it. In other words, you force an > > extra buffer copy. I'm not sure how important this is, but it would be > > nice to avoid it. > > Yeah. It makes the code harder though. For now I just want to add some > bits that > a) solve my problem at this point, so I can run my app based on the next > gstreamer release > b) are extendable to do the right thing in the future, without having to > change the API. I think it makes the code easier. Any plugin that wants to modify a buffer simply calls gst_buffer_make_writable() on it first, and has nothing further to think about. At no point do coders have to worry about making copies of buffers readonly, or whatever. Making tee return readonly buffers is a quick hack IMHO. Quick hacks take longer to sort out in the long run. > I'd like to do this quickly and before the next release, so I can update > all the filters I've written to take this into account, so they're > actually usable ;) I think it's always been implicit in the API that if a buffer is writable and is modified then other copies of it will be changed. Documenting this clearly, and how to deal with it if you need to have separate modifiable copies, is all that needs to be done. Making a helper function would be the best way of helping plugins to deal with it, and doesn't break compatability with old plugins (though new plugins won't work with the old core). Add that now, too, I say. I'll shut up now, since I'm not about to write some code. ;-) -- Richard |
From: Cameron H. <cam...@xd...> - 2002-08-29 23:44:43
|
Once upon a time Thomas Vander Stichele said... > > Heh, it's great that you comment on the manual ! I've been so wrapped up > in internals that I rarely bother to check it anymore, I should do that > more often. Thanks for replying, and let us know if you find anything in > there that's not clear or right so we can work on it. I've found a couple of things so far that I've started to fix myself. I've got the cvs version checked out. I'll generate some patches soon and mail them to this list for checking. How should I generate patches? Currently I'm doing "cvs diff -u gstreamer/docs/manual" which generates filenames relative to the cvs root. Since I'm new to glib too I've got a question about that. I've noticed some slight inconsistency in the manual with signal functions. One function takes the data pointer as a gpointer and cast it to the correct type, and another takes it as the actual type (GstElement * in this case think). What is the correct or best way to define signal functions? |
From: Andy W. <wi...@po...> - 2002-08-30 03:32:04
|
On Fri, 30 Aug 2002, Cameron Hutchison wrote: > I've got the cvs version checked out. I'll generate some patches soon > and mail them to this list for checking. Great! > How should I generate patches? > Currently I'm doing "cvs diff -u gstreamer/docs/manual" which generates > filenames relative to the cvs root. That's right. As long as it's a diff -u it's fine; the patchlevel argument to patch (-pN) can strip off dirs. > Since I'm new to glib too I've got a question about that. I've noticed > some slight inconsistency in the manual with signal functions. One > function takes the data pointer as a gpointer and cast it to the correct > type, and another takes it as the actual type (GstElement * in this case > think). What is the correct or best way to define signal functions? It doesn't really matter. If you declare it as a gpointer and cast it, you can take advantage of gobject's type checking; this can be good if you mix up your signal arguments somehow (happens to me more often than I'd like to admit ;). If you go ahead and declare it, you save yourself some typing. It adds up to the same thing, though, as all pointers are of course the same size. cheers, wingo. |
From: Joshua N P. <vi...@po...> - 2002-08-30 07:32:52
|
On Thu, Aug 29, 2002 at 04:59:48PM +0200, Thomas Vander Stichele wrote: > The other thing that's needed then is the change to tee I made, which I > think is reasonable; if tee sends out the same buffer to more than one > pad, all of those buffers should be read-only. Eh? i would expect tee to emit the original stream unchanged (read/write) and any copies of the stream marked read-only. So the first pad connect to a tee is special (r/w), but doesn't that make sense? -- Victory to the Divine Mother!! after all, http://sahajayoga.org http://why-compete.org |
From: Erik W. <om...@te...> - 2002-08-30 07:43:45
|
On Fri, 30 Aug 2002, Joshua N Pritikin wrote: > Eh? i would expect tee to emit the original stream unchanged (read/write) > and any copies of the stream marked read-only. So the first pad connect > to a tee is special (r/w), but doesn't that make sense? Ideally, every element that plans to modify a buffer in place will call gst_buffer_copy_on_write(). The default behavior of the tee element is then to simply increment the refcount for each extra output and send the same buffer to all downstream peers. The buffer is then implicitly (explicitly?) read-only until the refcount drops back to 1. As elements need to modify their "copies" in place, they CoW them at that point, so the buffers only get copied when necessary. The efficiency of the _copy_on_write() function is pretty high in the usual case, and if we deem it important we can make it a macro/inline-func with a slowpath, where the macro only calls the slowpath function when the refcount is greater than 1, bringing that one check inline. It's worth having the option for the tee to simply copy every buffer no matter what, but that would be very rarely used, since the _copy_on_write() calls would occur anyway. Actually calling _copy_on_write() is yet another in a relatively long list of things that all plugin-writers should check off for each plugin they write. Is there any such list anywhere in CVS that can be worked from? Erik Walthinsen <om...@te...> - System Administrator __ / \ GStreamer - The only way to stream! | | M E G A ***** http://gstreamer.net/ ***** _\ /_ |