On Mon, Sep 16, 2013 at 5:34 PM, Yonit Halperin <yhalperi@xxxxxxxxxx> wrote:
> Hi,
>
> 'force' means: wait till there is no pipe item that references the surface.
> If force = FALSE, try to release any such pipe item, but as long as it
> doesn't require blocking.
>
>
> On 09/16/2013 09:00 AM, Marc-André Lureau wrote:
>>
>> Hi,
>>
>> Since I don't understand the "force" argument, it would be easier to
>> review to keep current behaviour and just call
>> red_wait_outgoing_item() at the end.
>>
>> To me the meaning of "force" argument is rather "do not wait if dep".
>> When a drawable depend on the surface:
>> - force == false: return immediately when found
>> - force == true: wait for the first found item to be sent (I guess it
>> is the last to be sent, but isn't it supposed to be sent before the
>> one that depends on it?)
>
> red_clear_surface_drawables_from_pipe(surface=S) scans the pipe from the
> newest item to the oldest item (the first to be sent). It removes the newer
> pipe items that are directly drawn over surface S, till it finds an item
> that depends on S (i.e., S is its src/mask/brush). Such item might require
> that older pipe items that are drawn over S will be rendered on the client
> side. Thus, if force=TRUE, we wait till the newest item that depends on S is
> sent to the client.
Ok, that's pretty clear in the code. However why is it called "force"?
It's rather "wait if used" (trying to find an appropriate name).
> red_wait_outgoing_item() is called when a dependent item is not found, for
> case that the last item that was dequeued from the pipe, and is currently
> during sending, refers to surface S (we could have had a method that returns
> the currently sent item, and check it, but we don't have one yet)...
> I can add this comments to the code if it helps.
Ok, I still think it would be simpler if it wouldn't depend on this
weird "if (force)" conditionnal block. Ie, do it unconditionnally,
like before, with a comment explaining why it is necessary. (although
probably this wait should be done before or during
pipe_remove_and_release..)
>>
>> If you could clarified the above, and comment it in the code that
>> would be helpful! :)
>>
>>
>> On Thu, Sep 12, 2013 at 10:04 PM, Yonit Halperin <yhalperi@xxxxxxxxxx>
>> wrote:
>>>
>>> (1) merge 'force' and 'wait_for_outgoing_item' to one parameter.
>>> 'wait_for_outgoing_item' is a derivative of 'force'.
>>> (2) move the call to red_wait_outgoing_item to
>>> red_clear_surface_drawables_from_pipe
>>> ---
>>> server/red_worker.c | 20 +++++++++++---------
>>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>> index 0c611d0..e9a7fa1 100644
>>> --- a/server/red_worker.c
>>> +++ b/server/red_worker.c
>>> @@ -2064,22 +2064,24 @@ static void
>>> red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
>>>
>>> if (item) {
>>> red_channel_client_wait_pipe_item_sent(&dcc->common.base,
>>> item);
>>> + } else if (force) {
>>> + /*
>>> + * in case that the pipe didn't contain any item that is
>>> dependent on the surface, but
>>> + * there is one during sending.
>>> + */
>>> + red_wait_outgoing_item(&dcc->common.base);
>>> }
>>> }
>>>
>>> -static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
>>> int surface_id,
>>> - int force, int wait_for_outgoing_item)
>>> +static void red_clear_surface_drawables_from_pipes(RedWorker *worker,
>>> + int surface_id,
>>> + int force)
>>> {
>>> RingItem *item, *next;
>>> DisplayChannelClient *dcc;
>>>
>>> WORKER_FOREACH_DCC_SAFE(worker, item, next, dcc) {
>>> red_clear_surface_drawables_from_pipe(dcc, surface_id, force);
>>> - if (wait_for_outgoing_item) {
>>> - // in case that the pipe didn't contain any item that is
>>> dependent on the surface, but
>>> - // there is one during sending.
>>> - red_wait_outgoing_item(&dcc->common.base);
>>> - }
>>> }
>>> }
>>>
>>> @@ -4248,7 +4250,7 @@ static inline void red_process_surface(RedWorker
>>> *worker, RedSurfaceCmd *surface
>>> otherwise "current" will hold items that other drawables may
>>> depend on, and then
>>> red_current_clear will remove them from the pipe. */
>>> red_current_clear(worker, surface_id);
>>> - red_clear_surface_drawables_from_pipes(worker, surface_id,
>>> FALSE, FALSE);
>>> + red_clear_surface_drawables_from_pipes(worker, surface_id,
>>> FALSE);
>>> red_destroy_surface(worker, surface_id);
>>> break;
>>> default:
>>> @@ -10921,7 +10923,7 @@ static inline void destroy_surface_wait(RedWorker
>>> *worker, int surface_id)
>>> otherwise "current" will hold items that other drawables may
>>> depend on, and then
>>> red_current_clear will remove them from the pipe. */
>>> red_current_clear(worker, surface_id);
>>> - red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE,
>>> TRUE);
>>> + red_clear_surface_drawables_from_pipes(worker, surface_id, TRUE);
>>> }
>>>
>>> static void dev_destroy_surface_wait(RedWorker *worker, uint32_t
>>> surface_id)
>>> --
>>> 1.8.1.4
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>>
>>
>>
>
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel