GNOME Bugzilla – Bug 688251
xvimagesink: don't leave the ref to the pool in case of re-negotiation
Last modified: 2012-11-26 09:14:28 UTC
Created attachment 228885 [details] [review] xvimagesink: don't leave the ref to the pool in case of re-negotiation The xvimagesink should always keep a reference to the bufferpool. If renegotiation happens, sink is creating a new pool but doesn't keeping the reference to that at the moment. Attaching a patch to fix this:
This is not correct. The sink only keeps a ref to its pool when it gets a caps event and when it actually negotiates to the format. In the allocation query, the sink proposes a bufferpool. If the requested format is the same as the currently negotiated format, it can propose that pool, otherwise it needs to make a new one.
sreerenj: btw, it is always helpful to describe the context / situation / bug that led you to create a patch (what pipeline were you using? What did you do? What happened? etc.) - anything that might give us some clues where the actual problem lies - or did you just happen to read the code and propose this?
(In reply to comment #1) > This is not correct. The sink only keeps a ref to its pool when it gets a caps > event and when it actually negotiates to the format. > > In the allocation query, the sink proposes a bufferpool. If the requested > format is the same as the currently negotiated format, it can propose that > pool, otherwise it needs to make a new one. so you meant the the sink doesn't need to keep the ref to the pool, even though it is a new pool created in propose_allocation ? Then the sink is keeping ref to a pool(created in set_caps) which is not using by the upstream elements,,right?
(In reply to comment #3) > (In reply to comment #1) > > This is not correct. The sink only keeps a ref to its pool when it gets a caps > > event and when it actually negotiates to the format. > > > > In the allocation query, the sink proposes a bufferpool. If the requested > > format is the same as the currently negotiated format, it can propose that > > pool, otherwise it needs to make a new one. > > so you meant the the sink doesn't need to keep the ref to the pool, even though > it is a new pool created in propose_allocation ? Then the sink is keeping ref > to a pool(created in set_caps) which is not using by the upstream > elements,,right? Yes, correct.
okay,,is that scenario right??seems like tool pool active at the same time !!
(In reply to comment #5) > okay,,is that scenario right??seems like tool pool active at the same time !! hm,,sorry for the spelling mistake :( okay,,is that scenario right??seems like two pools active at the same time !!
(In reply to comment #6) > (In reply to comment #5) > > okay,,is that scenario right??seems like tool pool active at the same time !! > > hm,,sorry for the spelling mistake :( > okay,,is that scenario right??seems like two pools active at the same time !! What could be done is disable the pool from setcaps if a buffer with a different pool arrives.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > okay,,is that scenario right??seems like tool pool active at the same time !! > > > > hm,,sorry for the spelling mistake :( > > okay,,is that scenario right??seems like two pools active at the same time !! > > What could be done is disable the pool from setcaps if a buffer with a > different pool arrives. you mean we have to unref the dirty pool when the buffer received in show_frame() ?... hm,,i am again bit confused about the current implementaion for eg: there are two cases (the sink is creating pool in both cases) case1: If there is no renegotiation happened in the pipeline: the pool will be active even though the upstream elements drop the ref to the pool since the sink is holding the reference to the pool case2: If renegotiation happens once: then upstream element can drop ref to the pool and which may lead to pool-deactivation since the sink doesn't hold any ref to the pool. is this scenario is correct?
(In reply to comment #2) > sreerenj: btw, it is always helpful to describe the context / situation / bug > that led you to create a patch (what pipeline were you using? What did you do? > What happened? etc.) - anything that might give us some clues where the actual > problem lies - or did you just happen to read the code and propose this? I was looking through the xvimagesink code to implement the same behavior for a different sink and felt that the xvsink code is incorrect.
Can you reopen the bug based on comment 5,6 and 7 ?? or the better way is to unref the dirty pool in propose_allocation itself... propose_allocation: if new_poo_created_in_propose_allocation then unref the dirty pool
(In reply to comment #10) > Can you reopen the bug based on comment 5,6 and 7 ?? > > or the better way is to unref the dirty pool in propose_allocation itself... > propose_allocation: > if new_poo_created_in_propose_allocation then > unref the dirty pool No, the pool in propose_allocation has nothing to do with the pool used for rendering. The pool is a *proposal* based on the caps of the upstream element that is free to use the pool or not. The current code is correct.