Questions about experimental Spice compositor rebase

Tue Mar 1 16:16:28 UTC 2016


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.
> 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?
>
>>
>> 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/20160301/dd251f39/attachment-0001.html>