Add `auto`, `hide` and `always` three background drawing mode support. #50

taoyuan · 2019-12-24T12:35:24Z

This PR extended raop_callback_s to provide two more callbacks: conn_init and conn_destroy.

So we can listen the connection activity to render background when first connection connected and remove background when last connection disconnected.

moda20 · 2019-12-24T12:39:16Z

So technically we can now not have to deal with the black screen and just render the incoming flow when it comes ?

taoyuan · 2019-12-24T12:42:14Z

This may be make #37 possible.

taoyuan · 2019-12-24T12:42:56Z

@moda20 It should be.

FD-

Thanks for your PR! I have a few suggestions for your changes before merging them:

It seems your IDE modified the indentation in a lot of places. I have now streamlined many project files to consistently use four spaces instead of tabs, so please rebase on that commit and remove your modifications to indentation. It’s difficult to figure out the actual changes you made since there are lots of line replacements highlighted that are really just the indentation.
For my use case, I still want the background to be visible independent of whether there is an active connection. Thus, please add a command line option for setting the background behavior. I suggest this to be an additional argument to the -b flag that indicates the desired background mode: "0": Don’t draw any background, "1": Draw background only while there’s an active connection, "2": Always draw background. Probably strings for the different modes would be better than numbers (and more in line with the audio sink selection), but I can’t come up with any short descriptive names. Let me know if you have any ideas!  For the case that the user chooses to always draw the background, please move the call to video_renderer_render_background into rpiplay.cpp, to the place immediately after the video renderer is initialised. That way, it's always rpiplay.cpp's responsibility to manage the background, instead of having it spread around in rpiplay.cpp and the renderer itself. This should also allow us to remove the background argument to the renderer initialisation.

FD- · 2020-01-02T10:36:49Z

lib/raop.h

@@ -28,19 +28,22 @@ extern "C" {


 typedef struct raop_s raop_t;
+typedef struct raop_conn_s raop_conn_t;


What's that for?

It is used in new added callbacks conn_init and conn_destory for generic usage. In terms of drawing background, it is not necessary.

What do you think?

Oh, I see, this is just the typedef. At first glance, I thought it was a global variable. Still, just for the background rendering, I'd prefer not to expose library internals. If we need it at a later point, we can still add it back in pretty easily. So, I'd suggest to remove the raop_conn_t argument from the two new callback functions.

Removed the conn parameter from conn_init and conn_destroy and the conn typedef in the latest commit

FD- · 2020-01-02T10:42:08Z

renderers/video_renderer_rpi.c

@@ -117,7 +133,7 @@ int video_renderer_init_decoder(video_renderer_t *renderer) {

    bcm_host_init();

-    if (renderer->background) video_renderer_draw_background();


For my use case, I still want the background to be drawn independent of whether there is an actual connection. Thus, please add a command line option for toggling the background mode, more details in the discussion thread.

For the case that the user chooses to always draw the background, please move the call to video_renderer_render_background into rpiplay.cpp, to the place immediately after the video renderer is initialised. That way, it's always rpiplay.cpp's responsibility to manage the background, instead of having it spread around in rpiplay.cpp and the renderer itself. This should also allow us to remove the background argument to the renderer initialisation.

In latest commit, there is a new function video_renderer_update_background will be called in video_renderer_init to support always background drawing mode.

rpiplay.cpp is responsable to call video_renderer_update_background in conn_init and conn_destroy that seems only can be set to raop_t instance in start_server of rpiplay.cpp.

If there is better way to move all background details to video renderer, please tell me, I will do it.

FD- · 2020-01-02T11:01:50Z

renderers/video_renderer_rpi.c

@@ -59,36 +59,52 @@ struct video_renderer_s {
    uint64_t input_frames;
 };

+DISPMANX_ELEMENT_HANDLE_T background = 0;


Please move this into the video_renderer_s struct.

It has been done in latest commit

taoyuan · 2020-01-03T03:42:19Z

Thanks for your review. I will take some time to make some explanations and changes these days for this PR.

taoyuan · 2020-01-04T02:42:47Z

Thanks for your PR! I have a few suggestions for your changes before merging them:

It seems your IDE modified the indentation in a lot of places. I have now streamlined many project files to consistently use four spaces instead of tabs, so please rebase on that commit and remove your modifications to indentation. It’s difficult to figure out the actual changes you made since there are lots of line replacements highlighted that are really just the indentation.

It may be that I see inconsistencies in the indentation and habitually reformat it. In future collaborations, I will keep the original format as much as possible, and only submit logic change code.

By the way, we can use (EditorConfig)[https://editorconfig.org] that helps maintain consistent coding styles for multiple developers working on the same project across various editors and IDEs.

For my use case, I still want the background to be visible independent of whether there is an active connection. Thus, please add a command line option for setting the background behavior. I suggest this to be an additional argument to the -b flag that indicates the desired background mode: "0": Don’t draw any background, "1": Draw background only while there’s an active connection, "2": Always draw background. Probably strings for the different modes would be better than numbers (and more in line with the audio sink selection), but I can’t come up with any short descriptive names. Let me know if you have any ideas!  For the case that the user chooses to always draw the background, please move the call to video_renderer_render_background into rpiplay.cpp, to the place immediately after the video renderer is initialised. That way, it's always rpiplay.cpp's responsibility to manage the background, instead of having it spread around in rpiplay.cpp and the renderer itself. This should also allow us to remove the background argument to the renderer initialisation.

I have refactored -b flag to add auto, hide and always to support three background modes. And also refactor video_renderer_s->background to int type:

0: no background
1: draw background only while there's an active connection
2: always draw background

FD-

Thanks for your modifications!

Before I can merge this, please also correct the indentation to match the four spaces that are used in most of the rest of the project.

EditorConfig seems like a good way to automatically maintain a coding style. If you have experience setting it up, please submit a separate PR for that. Please note that since this project includes files from various different origins, they adhere to different coding styles at the moment. I tried to streamline them all, but some make use of kind of ASCII-art formatting that would look weird if imposing a different coding style. Ideally, they'd have to manually be corrected.

FD- · 2020-01-04T10:04:49Z

rpiplay.cpp

@@ -36,13 +36,13 @@
 #define VERSION "1.2"

 #define DEFAULT_NAME "RPiPlay"
-#define DEFAULT_SHOW_BACKGROUND true
+#define DEFAULT_BACKGROUND 1


Please make it default to always/on, to keep backwards-compatibility.

It has been done.

FD- · 2020-01-04T14:46:02Z

Great, thanks! Please squash your commits so we don't have all these small commits cluttering the commit history and I'll happily merge it!

taoyuan · 2020-01-04T16:25:04Z

The commits has been squashed and renamed the name of the PR.

FD- · 2020-01-04T17:47:58Z

Merged into master and working fine. Thanks again for your contribution!

moda20 · 2020-01-05T09:52:07Z

is the compilation process still the same ? how to choose between the new parameters when launching ?

horge · 2020-01-05T19:20:00Z

@moda20 compilation process has not changed. take a look at the updated README.md usage section for new parameters

FD- requested changes Jan 2, 2020

View reviewed changes

taoyuan force-pushed the master branch from 8108268 to 5ea76c3 Compare January 3, 2020 03:33

FD- reviewed Jan 4, 2020

View reviewed changes

Add auto, hide and always three background drawing mode support.

a430943

taoyuan force-pushed the master branch from 8ba7771 to a430943 Compare January 4, 2020 16:13

taoyuan changed the title ~~Render background only when have connections if background enabled~~ Add auto, hide and always three background drawing mode support. Jan 4, 2020

FD- merged commit cd69266 into FD-:master Jan 4, 2020

FD- mentioned this pull request Jan 4, 2020

Output only while casting #37

Closed

FD- mentioned this pull request Jan 20, 2020

Idle screen support instead of pitch black screen #29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `auto`, `hide` and `always` three background drawing mode support. #50

Add `auto`, `hide` and `always` three background drawing mode support. #50

taoyuan commented Dec 24, 2019 •

edited

Loading

moda20 commented Dec 24, 2019

taoyuan commented Dec 24, 2019

taoyuan commented Dec 24, 2019

FD- left a comment

FD- Jan 2, 2020

taoyuan Jan 3, 2020

FD- Jan 3, 2020

taoyuan Jan 3, 2020

taoyuan Jan 4, 2020

FD- Jan 2, 2020

FD- Jan 2, 2020

taoyuan Jan 4, 2020

FD- Jan 2, 2020

taoyuan Jan 4, 2020

taoyuan commented Jan 3, 2020

taoyuan commented Jan 4, 2020

FD- left a comment

FD- Jan 4, 2020

taoyuan Jan 4, 2020

FD- commented Jan 4, 2020

taoyuan commented Jan 4, 2020 •

edited

Loading

FD- commented Jan 4, 2020

moda20 commented Jan 5, 2020

horge commented Jan 5, 2020

		@@ -28,19 +28,22 @@ extern "C" {


		typedef struct raop_s raop_t;
		typedef struct raop_conn_s raop_conn_t;

		@@ -117,7 +133,7 @@ int video_renderer_init_decoder(video_renderer_t *renderer) {

		bcm_host_init();

		if (renderer->background) video_renderer_draw_background();

Add auto, hide and always three background drawing mode support. #50

Add auto, hide and always three background drawing mode support. #50

Conversation

taoyuan commented Dec 24, 2019 • edited Loading

moda20 commented Dec 24, 2019

taoyuan commented Dec 24, 2019

taoyuan commented Dec 24, 2019

FD- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoyuan commented Jan 3, 2020

taoyuan commented Jan 4, 2020

FD- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FD- commented Jan 4, 2020

taoyuan commented Jan 4, 2020 • edited Loading

FD- commented Jan 4, 2020

moda20 commented Jan 5, 2020

horge commented Jan 5, 2020

Add `auto`, `hide` and `always` three background drawing mode support. #50

Add `auto`, `hide` and `always` three background drawing mode support. #50

taoyuan commented Dec 24, 2019 •

edited

Loading

taoyuan commented Jan 4, 2020 •

edited

Loading