[Spice-devel] [PATCH spice-streaming-agent 1/2] Wait to have some client before initialising capture

[Christophe de Dinechin]

> On 1 Dec 2017, at 17:56, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> On Tue, 2017-11-21 at 09:49 +0000, Frediano Ziglio wrote:
>>> This saves some resources if no client are connected.
>>> Also will allow to get a better capture engine taking into account
>>> client supported codecs.
>>> 
>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>> ---
>>> src/spice-streaming-agent.cpp | 14 +++++++-------
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
>>> agent.cpp
>>> index 23b9768..20f2925 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -343,10 +343,6 @@ static void cursor_changes(Display *display, int
>>> event_base)
>>> static void
>>> do_capture(const char *streamport, FILE *f_log)
>>> {
>>> -    std::unique_ptr<FrameCapture>
>>> capture(agent.GetBestFrameCapture());
>>> -    if (!capture)
>>> -        throw std::runtime_error("cannot find a suitable capture
>>> system");
>>> -
>>>     streamfd = open(streamport, O_RDWR);
>>>     if (streamfd < 0)
>>>         // TODO was syslog(LOG_ERR, "Failed to open %s: %s\n",
>>> streamport, strerror(errno));
>>> @@ -361,9 +357,16 @@ do_capture(const char *streamport, FILE *f_log)
>>>             }
>>>         }
>>> 
>>> +        if (quit)
>>> +            return;
>>> +
>> 
>> 
>> This hunk looks unrelated, no? Also, returning directly here bypasses
>> the 'done:' section at the end of the function. That doesn't seem
>> correct.
>> 
> 
> It is, previously the capture was build at the beginning so
> when quit was set code exited directly, now it won't.
> 
>> (by the way, since we're using c++ here, couldn't we do the cleanup in
>> some sort of destructor rather than using a goto? )
>> 
> 
> In the current code there's no much difference, the device is closed
> anyway. There's another series (which currently seems stuck) that is
> providing the proper destructor.

Will try to unstuck that stuff this week.

> 
>> 
>> 
>>>         syslog(LOG_INFO, "streaming starts now\n");
>>>         uint64_t time_last = 0;
>>> 
>>> +        std::unique_ptr<FrameCapture>
>>> capture(agent.GetBestFrameCapture());
>>> +        if (!capture)
>>> +            throw std::runtime_error("cannot find a suitable capture
>>> system”);
>>> +

Would it make sense (in a later patch) to have the error message generated
by GetBestFrameCapture, which might allow us to get a better sense of 
why a specific plugin did not generate the capture?

>>>         while (!quit && streaming_requested) {
>>>             if (++frame_count % 100 == 0) {
>>>                 syslog(LOG_DEBUG, "SENT %d frames\n", frame_count);
>>> @@ -410,9 +413,6 @@ do_capture(const char *streamport, FILE *f_log)
>>>                 syslog(LOG_ERR, "FAILED to read command\n");
>>>                 goto done;
>>>             }
>>> -            if (!streaming_requested) {
>>> -                capture->Reset();
>>> -            }
>>>         }
>>>     }
>>> 
>> 
>> 
>> Otherwise seems fine to me
>> 
>> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
>> 

Acked-by: Christophe de Dinechin <dinechin at redhat.com>

> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel