hwcomposer: Want so contribute and cleanup

[Molytho]

I was investigating waydroid/waydroid#1875 and was looking at hwcomposer.
To me the code in hwcomposer.cpp looks like it could get some cleanup
I mean things like:

1. wp_presentation_feedback being stored in struct buffer which is wrong compared to the wayland protocol associating wp_presentation_feedback with wl_surface
2. Misuse of wl_surface_damage. The coordinates should be surface-local but buffer-local coordinates are given here

   android_hardware_waydroid/hwcomposer/hwcomposer.cpp

   Line 678 in 13b570b

   wl_surface_damage(pdev->display->cursor_surface, 0, 0, buf->width, buf->height);

3. Some functions are huge (especially hwc_set) which could be improved by factoring out some functionality to new functions

I currently have much free time and would like to improve code quality.
My questions is: Is a pull request refactoring code reviewed and accepted? I would like to avoid putting in the work just so that it is rejected.

After I finished refactoring I want to implement HWC_DEVICE_API_VERSION_1_5 because of its damage tracking.

Side note on your fix for waydroid/waydroid#1875 (13b570b):
I don't think that the HWC_GEOMETRY_CHANGED flag is really the right thing to check for here.
The underlying issue is that a new buffer might get the same buffer_handle_t as a buffer we saw previously which is technically also possible without HWC_GEOMETRY_CHANGED being set.
I suggest something like this Molytho@3fe3386

[aleasto]

Yeah I agree that hwcomposer is in poor shape!

1. wp_presentation_feedback being stored in struct buffer which is wrong compared to the wayland protocol associating wp_presentation_feedback with wl_surface

I'm unfamiliar with this at the moment

2. Misuse of wl_surface_damage. The coordinates should be surface-local but buffer-local coordinates are given here

I'm aware, but it's also probably fair to assume that all compositors today implement wl_surface_damage_buffer

3. Some functions are huge (especially hwc_set) which could be improved by factoring out some functionality to new functions

Yes!

My questions is: Is a pull request refactoring code reviewed and accepted? I would like to avoid putting in the work just so that it is rejected.

Definitely! There was an attempt by another first-time contributor in #40 but it introduced regressions and it never went anywhere.

Please start small and split the changes logically in reviewable commits.

I don't think that the HWC_GEOMETRY_CHANGED flag is really the right thing to check for here.
The underlying issue is that a new buffer might get the same buffer_handle_t as a buffer we saw previously which is technically also possible without HWC_GEOMETRY_CHANGED being set.

From hwcomposer.h:

            /* When compositionType is HWC_FRAMEBUFFER, HWC_OVERLAY,
             * HWC_FRAMEBUFFER_TARGET, this is the handle of the buffer to
             * compose. This handle is guaranteed to have been allocated
             * from gralloc using the GRALLOC_USAGE_HW_COMPOSER usage flag.
             * If the layer's handle is unchanged across two consecutive
             * prepare calls and the HWC_GEOMETRY_CHANGED flag is not set
             * for the second call then the HWComposer implementation may
             * assume that the contents of the buffer have not changed. */
            buffer_handle_t handle;

[Molytho]

My questions is: Is a pull request refactoring code reviewed and accepted? I would like to avoid putting in the work just so that it is rejected.

Definitely! There was an attempt by another first-time contributor in #40 but it introduced regressions and it never went anywhere.

Please start small and split the changes logically in reviewable commits.

Alright. I will do some refactoring then.

I don't think that the HWC_GEOMETRY_CHANGED flag is really the right thing to check for here.
The underlying issue is that a new buffer might get the same buffer_handle_t as a buffer we saw previously which is technically also possible without HWC_GEOMETRY_CHANGED being set.

From hwcomposer.h:

            /* When compositionType is HWC_FRAMEBUFFER, HWC_OVERLAY,
             * HWC_FRAMEBUFFER_TARGET, this is the handle of the buffer to
             * compose. This handle is guaranteed to have been allocated
             * from gralloc using the GRALLOC_USAGE_HW_COMPOSER usage flag.
             * If the layer's handle is unchanged across two consecutive
             * prepare calls and the HWC_GEOMETRY_CHANGED flag is not set
             * for the second call then the HWComposer implementation may
             * assume that the contents of the buffer have not changed. */
            buffer_handle_t handle;

I'm aware of that but I can think of following scenario:

1. SurfaceFlinger creates buffer with handle 1 and attaches it to layer 1.
2. SurfaceFlinger creates new buffer with handle 2 and attaches it to layer 1 replacing buffer 1.
   This does not require SurfaceFlinger to set HWC_GEOMETRY_CHANGED if the buffer is the only thing changing. (So if dimension, crop, etc. stay the same)
   From hwcomposer.h:

    /*
     * HWC_GEOMETRY_CHANGED is set by SurfaceFlinger to indicate that the list
     * passed to (*prepare)() has changed by more than just the buffer handles
     * and acquire fences.
     */
    HWC_GEOMETRY_CHANGED = 0x00000001,

3. SurfaceFlinger destroys buffer 1, creates a new buffer with different geometry which also receives handle 1.
   At this point we still cached a wrong buffer for handle 1.
   I now see that your change also works if SurfaceFlinger attaches the new buffer 1 to layer 1 again because the geometry changed thus HWC_GEOMETRY_CHANGED is set.
   Purely theoretical (but highly unlikely) SurfaceFlinger could attach the new buffer 1 to some layer 2 and if layer 2 has matching geometry SurfaceFlinger would not need to set HWC_GEOMETRY_CHANGED.

I think my whole point was that this check in get_wl_buffer just seems odd to me.

android_hardware_waydroid/hwcomposer/hwcomposer.cpp

Lines 213 to 224 in 13b570b

	if (it != pdev->display->buffer_map.end()) {
	if (it->second->isShm) {
	if (width != it->second->width || height != it->second->height) {
	destroy_buffer(it->second);
	pdev->display->buffer_map.erase(it);
	} else {
	update_shm_buffer(pdev->display, it->second);
	return it->second;
	}
	} else
	return it->second;
	}

[Molytho]

Another question:
Currently hwcomposer's code is mostly written in c-style. There is few usage of classes (constructors, destructors, methods).
Is keeping it c-like preferred or can I convert it to proper c++ using classes?

[aleasto]

C++ up to gnu++14 is fine

[aleasto]

C++ up to gnu++14 is fine

FYI I've actually updated all of our older branches (down to Android 9) to enable C++17.

I'm marking this issue as resolved. Thank you for your contributions!