Questions about experimental Spice compositor rebase

Fabio Fantoni fabio.fantoni at m2r.biz
Thu Mar 3 15:40:01 UTC 2016 

Il 01/03/2016 17:16, Yury Shvedov ha scritto:
>
>
> On 03/01/2016 04:56 PM, Fabio Fantoni wrote:
>> Il 29/02/2016 21:51, Yury Shvedov ha scritto:
>>> Hi, Fabio!
>>>
>>> I managed to compile and run compositor-spice with the latest weston 
>>> master branch. See last commit 
>>> <https://github.com/ein-shved/compositor-spice/commit/585cbd281fd4c4685df83d22d60ec5670479b474>.
>>>
>>> Here, how I did it:
>>>
>>> export PREFIX=$HOME/usr #setup prefix variable. You can choose 
>>> another or use default one
>>>
>>> sh autogen.sh --prefix=$PREFIX --disable-weston-launch 
>>> --enable-spice-compositor #configure with different preffix, 
>>> spice-compositor and without weston-launch to install as normal user
>>>
>>> make -j8 && make install && ./src/weston --backend=spice-backend.so 
>>> #compile project with 8 threads, install modules and run weston with 
>>> spice-compositor.
>>>
>>>
>>> On another terminal run
>>> spicy --display=:0 -h localhost -p 5912 # to connect to spice server.
>>>
>>> Please try it by yourself and let me know the results!
>>>
>>> This is only the first step. Next we need to remove all unnecessary 
>>> code, change backend to good weston coding style, move to new spice 
>>> API and check it with valgrind and unit-tests.
>>>
>>> After that we will think about optimization. Firstly, make all of it 
>>> like in RDP.
>>
>> Thanks, also today I not have enough free-time for test it but at 
>> maximum I'll do it in the weekend.
>> I'll also try it in lan changing listen from localhost to '0.0.0.0' 
>> (any) and in the weekend I'll try to make host and port configurable 
>> with weston.ini.
> I suppose, weston_config handles configuration from both argument list 
> and weston.ini. So, regarding to compositor-spice.c:349, you can 
> configure them with *host* and *port* parameters. Maybe we should use 
> weston_backend_config? Anyway this is the point to start from.

host and port parameter are working, added also in documentation

>> After probably I'll also try to add image compression and 
>> authentication (user and password only initially) support.
> Ok, till then I hope I'll upgrade code to the new Spice API.
>> About repository keep the method you prefer, I'll keep all changes 
>> ready for upstream post for review in another branch, for example now 
>> I did fast a commit based on your latest changes with a small 
>> description (to improve):
>> https://github.com/Fantu/compositor-spice/commit/56719743370e2387b75ca31434dfc3bbb9d735f0
>> (based on your commit 585cbd2 - Restore mouse motion)
>> When will be enough good we'll start to post it to mailing list (with 
>> git format-patch and git send-email or different if required) for review.
> I think, now I understand what are you doing. I cant understand, why 
> are you keeping changes ready for upstream code? Why not to do it when 
> it became realy needed to post it to mailing list?

It is not a problemfor me, I do it quickly.
I did very fast some small commits:
https://github.com/Fantu/compositor-spice/commits/rebase/spice-1.10
Based on latest stable to avoid regression not related to spice.
I did fast test connecting from lan computer and is working.
You should able to cherry-pick all commits after "Add Spice compositor" 
without problem if the commits are ok for you.
In the weekend probably I'll add other spice features support.


>>
>>>
>>> On 02/29/2016 05:07 PM, Yury Shvedov wrote:
>>>>
>>>>
>>>> On 02/29/2016 04:39 PM, Fabio Fantoni wrote:
>>>>> Il 29/02/2016 16:01, Yury Shvedov ha scritto:
>>>>>> Unfortunately, it is bit complex for me, to understand what are 
>>>>>> you trying to do and why do you dancing with diff's instead of 
>>>>>> just simple merge.
>>>>>
>>>>> Sorry for my bad english.
>>>> Don't worry, your English is pretty good. Not worse then my =).
>>>> You didn't fully explain what are you trying to do. Can you?
>>>>> As you not rebase all your commits on top is difficult find all 
>>>>> your changes from upstream, so I did a quick diff from your branch 
>>>>> to the last upstream commit corresponding.
>>>> /git diff master spice/ will show you all the changes.
>>>>> Usually I keep do rebase of my patches (with git rebase -i) and 
>>>>> other patches in development to test always queued to upstream 
>>>>> commits to make it easier and faster update them, have it ready to 
>>>>> post upstream for review any version and add the new upstream 
>>>>> commits until my patches are accepted upstream.
>>>>> For example: 
>>>>> https://github.com/Fantu/Xen/commits/rebase/m2r-testing-4.6
>>>>> Probably will be good also for your project (I can do it in newer 
>>>>> branch).
>>>> I don't suppose myself the master of git but in my opinion using 
>>>> git rebase is a bad practice:
>>>>
>>>>   * rebase can rewrite the history. It can take the part of chain
>>>>     and move fully in another place. While merge doesn't changes
>>>>     history. It simply creates new commit on top of two branches
>>>>   * with rebase in case of conflict you have to resolve them in
>>>>     many commits, while with merge you need to resolve conflict
>>>>     once at the top
>>>>   * sometimes you need to perform code changing while action. In
>>>>     rebase you have to do it in several commits, while with merge -
>>>>     only in one
>>>>   * rolling back the rebase could be really painful
>>>>   * I already had problems with rebase, so now I use it only when
>>>>     it is realy needed and when I can't use merge (never).
>>>>
>>>>
>>>>>
>>>>> Look other answer/questions below please.
>>>>>
>>>>>>
>>>>>> Anyway, I gave you an edit-access to my repository. If you want, 
>>>>>> you can work with it directly on your own branch. I hope, this 
>>>>>> will make things simpler for you.
>>>>>>
>>>>>> See answers in quote.
>>>>>>
>>>>>> On 02/29/2016 03:32 PM, Fabio Fantoni wrote:
>>>>>>> Il 29/02/2016 12:26, Yury Shvedov ha scritto:
>>>>>>>> Hi, Fabio!
>>>>>>>>
>>>>>>>> Take look at my latest commit It now merged with latest master 
>>>>>>>> version and successfully compiles with ./configure 
>>>>>>>> --enable-spice-compositor.
>>>>>>>> But unfortunately it doesn't work due to new spice API. I hope, 
>>>>>>>> this evening it will!.
>>>>>>>
>>>>>>> Thanks for your work about it.
>>>>>>> I make the new diff in other test branch:
>>>>>>> https://github.com/Fantu/compositor-spice/tree/test2
>>>>>>> And I have some questions:
>>>>>>>
>>>>>>> - src/Makefile.am was removed in newer weston and now unused, I 
>>>>>>> suppose
>>>>>>>
>>>>>>> to be removed
>>>>>>>
>>>>>> Did I fogot to do it in my repo? Oh yes! My bad! I will!
>>>>>>>
>>>>>>> - Makefile.am missed monitor renderer additions, must be added 
>>>>>>> or monitor renderer is not
>>>>>>>
>>>>>>> needed anymore?
>>>>>>>
>>>>>> Its doesn't used by spice, so if there no monitor renderer 
>>>>>> additions in original weston repo, then it is not needed anymore.
>>>>>
>>>>> Monitor renderer seems something added by you with this project:
>>>>> https://github.com/ein-shved/compositor-spice/commit/72072ed2671dd400068d48b4f5048855fb066938
>>>>> There isn't a commit description about, I not understand if it 
>>>>> something additional for sharing monitor like a new weston plugin 
>>>>> I saw (screen sharing) or it is different and required for spice 
>>>>> compositor.
>>>>> Can you do a small little explanation if possible please?
>>>>>
>>>> Ouch! I remembered now! I developed this instrument to spy on 
>>>> wayland clients in developing purposes. I shell move its code to 
>>>> another branch.
>>>>>>>
>>>>>>> - src/compositor-rdp.c: I suppose is not needed and not related 
>>>>>>> changes
>>>>>>>
>>>>>>> to be removed, right?
>>>>>>>
>>>>>> Why? It is just another part of weston. If you don't need it just 
>>>>>> don't pass --enable-rd-compositor to configure.
>>>>>
>>>>> I talked only about few lines changed by one of your commit.
>>>> Sorry for misunderstanding. Will take a look.
>>>>>
>>>>>>> - src/spice/Makefile.am: I suppose is unused now that thing are 
>>>>>>> added in
>>>>>>>
>>>>>>> Makefile.am, to be removed, right?
>>>>>>>
>>>>>> Yes, the same as src/Makefile.am
>>>>>>>
>>>>>>> Can be the monitor renderer missed/incomplete the cause of "run 
>>>>>>> test" failed?
>>>>>>>
>>>>>> I didn't try tests, so can't answer. Will look at evening.
>>>>>>>
>>>>>>> About spice-server api I did't found good docs to make update 
>>>>>>> simply and fast but with a fast search I found this xspice 
>>>>>>> (similar project for xorg instead) commit that probably can be 
>>>>>>> faster update some deprecrated spice functions: 
>>>>>>> https://cgit.freedesktop.org/xorg/driver/xf86-video-qxl/commit/?id=70884bd353c34c0be23c2b21eec320cd8c637f4f
>>>>>>>
>>>>>>> I don't have sufficent free time for try to change it and test 
>>>>>>> build/use today.
>>>>>>>
>>>>>> I spend much time for reading spice source code to understand its 
>>>>>> API far in 2013. To understand it you have to read sources as I. 
>>>>>> I remember that in fact spice protocol is - to say simple - 
>>>>>> drawing API. You can draw stuff from spice-server on 
>>>>>> spice-client's screen. Anyway we need to learn new api, reading 
>>>>>> example source code as I did.
>>>>>>
>>>>>> I did simple example <https://github.com/ein-shved/qxl-test> then 
>>>>>> to practice on spice API.
>>>>>>>
>>>>>>> After update to newer api I suppose will be good add also a 
>>>>>>> required spice-server version check in configure based on newer 
>>>>>>> api, I found this that seems will make fast see at what version 
>>>>>>> was added any api: 
>>>>>>> https://cgit.freedesktop.org/spice/spice/tree/server/spice-server.syms
>>>>>>>
>>>>>> Yes of course we will!
>>>>>>>
>>>>>>> Another important note if you don't know it, spice-server 
>>>>>>> recently is under heavy changes, latest version (0.13.0) is like 
>>>>>>> a "devel snapshot". Latest stable version that I think is good 
>>>>>>> to use also with this project for now is 0.12.6.
>>>>>>>
>>>>>> Yes, I don't. Is it possible for you to assemble all documents 
>>>>>> and links on this topic, you found?
>>>>>>>
>>>>>>> Thanks for any reply and sorry for my bad english.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> On 02/29/2016 12:22 PM, Daniel Stone wrote:
>>>>>>>>> Hi Fabio,
>>>>>>>>>
>>>>>>>>> On 27 February 2016 at 18:02, Fabio Fantoni 
>>>>>>>>> <fabio.fantoni at m2r.biz> wrote:
>>>>>>>>>> Hi, long time ago I saw an interesting project for weston, 
>>>>>>>>>> the spice
>>>>>>>>>> compositor:
>>>>>>>>>> https://github.com/ein-shved/compositor-spice
>>>>>>>>>> It is now abandoned because the developer has been involved 
>>>>>>>>>> in another
>>>>>>>>>> project.
>>>>>>>>>> As no other has continued it, despite my low knowledge and 
>>>>>>>>>> time I would try
>>>>>>>>>> to update, test and possibly improve it.
>>>>>>>>> Great!
>>>>>>>>>
>>>>>>>>>> I did a new branch with only 2 commit on top of latest 
>>>>>>>>>> upstream commit:
>>>>>>>>>> https://github.com/Fantu/compositor-spice/commits/test
>>>>>>>>>> and I tried to do a fast rebase on latest upstream commit 
>>>>>>>>>> (1.10) instead of
>>>>>>>>>> master (development branch) for decrease the risk regression 
>>>>>>>>>> on first
>>>>>>>>>> build/use tests:
>>>>>>>>>> https://github.com/Fantu/compositor-spice/commits/rebase/spice-1.10 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Solving conflict about configure and makefile parts I have 
>>>>>>>>>> some doubts (as
>>>>>>>>>> also reported in the description of each commit):
>>>>>>>>>> About first commit (Add Spice compositor)
>>>>>>>>>> https://github.com/Fantu/compositor-spice/commit/f589ab264e80d43fa0853770481b6ddcadf5505b 
>>>>>>>>>>
>>>>>>>>>> - in configure.ac some changes seems strange, including LIBS 
>>>>>>>>>> and CFLAGS that
>>>>>>>>>> seems "double"
>>>>>>>>> I think this can be removed. Usually setting LIBS/CFLAGS and
>>>>>>>>> foo_save_LIBS/foo_save_CFLAGS is used for an AC_CHECK_* call, 
>>>>>>>>> which
>>>>>>>>> relies on LIBS and CFLAGS already being set. I guess there may 
>>>>>>>>> have
>>>>>>>>> been a call here which has since been removed.
>>>>>>>>>
>>>>>>>>>> About the second commit (Monitor renderer)
>>>>>>>>>> https://github.com/Fantu/compositor-spice/commit/2632b8b8067e46ac69b5ad1bc2164d90ced5e19f 
>>>>>>>>>>
>>>>>>>>>> - Makefile things seems fully changed, tried to adapt them 
>>>>>>>>>> but I'm not sure
>>>>>>>>>> if I did it correct.
>>>>>>>>>> - Add -g to AM_CPPFLAGS in Makefile.am is really needed? not 
>>>>>>>>>> added for now
>>>>>>>>> No, this is a debugging feature only.
>>>>>>>>>
>>>>>>>>>> - add of "-Wl,--wrap=pixman_renderer_init" to LDFLAGS of many 
>>>>>>>>>> other backend
>>>>>>>>>> is really needed? not added for now, if needed is good 
>>>>>>>>>> understand why to add
>>>>>>>>>> it also to new things added since this start commit done 3 
>>>>>>>>>> years ago
>>>>>>>>> This should be solved in a different way if required.
>>>>>>>>>
>>>>>>>>>> - src/compositor-rdp.c changes is really needed? if not I'll 
>>>>>>>>>> remove them
>>>>>>>>>>
>>>>>>>>>> I also searched documentation about api and/or internal 
>>>>>>>>>> weston functions
>>>>>>>>>> changed any versions but I not found them.
>>>>>>>>> There is no documentation on the change, no.
>>>>>>>>>
>>>>>>>>> As you can see, several functions have changed:
>>>>>>>>>    - weston_output_finish_frame now takes a struct timespec 
>>>>>>>>> rather than
>>>>>>>>> an integer number of milliseconds (trivial conversion)
>>>>>>>>>    - the output repaint function now returns an integer 
>>>>>>>>> marking success
>>>>>>>>> or failure
>>>>>>>>>    - the compositor interface has now changed to 
>>>>>>>>> weston_backend, and
>>>>>>>>> you can see examples of the changes required in commit 954f183e
>>>>>>>>>
>>>>>>>>> Hope this helps: just pick out the warnings and errors one by 
>>>>>>>>> one, and
>>>>>>>>> try to figure them out - searching git commits for anything 
>>>>>>>>> relevant
>>>>>>>>> always helps - until you get something that builds.
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Daniel
>>>>>>>>
>>>>>>>> -- Kind Regards, Yury Shvedov
>>>>>>>
>>>>>>
>>>>>> -- Kind Regards, Yury Shvedov
>>>>>
>>>>
>>>> -- 
>>>> Kind Regards,
>>>> Yury Shvedov
>>>
>>> -- 
>>> Kind Regards,
>>> Yury Shvedov
>>
>
> -- 
> Kind Regards,
> Yury Shvedov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160303/59677764/attachment-0001.html>

More information about the wayland-devel mailing list