Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix documentation around --enable-graphics flag #224

Merged
merged 4 commits into from
Nov 29, 2024

Conversation

keirthana
Copy link
Collaborator

Fixes #216

@keirthana keirthana requested a review from a team as a code owner November 19, 2024 12:48
@keirthana keirthana requested review from morphis and adglkh November 19, 2024 12:48
explanation/gpus-instances.md Outdated Show resolved Hide resolved
explanation/gpus-instances.md Outdated Show resolved Hide resolved
Copy link
Contributor

@adglkh adglkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reading this page, another question came to mind: with the recently added CLI flag --enable-streaming to amc launch comman, the webrtc platform will be used in such cases. Since the webrtc platform automatically detects the GPU and adjusts rendering or video encoding capabilities (software vs hardware) accordingly, should we mention this on this page as well?

explanation/gpus-instances.md Outdated Show resolved Hide resolved
@keirthana
Copy link
Collaborator Author

While reading this page, another question came to mind: with the recently added CLI flag --enable-streaming to amc launch comman, the webrtc platform will be used in such cases. Since the webrtc platform automatically detects the GPU and adjusts rendering or video encoding capabilities (software vs hardware) accordingly, should we mention this on this page as well?

I have added a note because it is worth informing the user that there is another flag (--enable-streaming) that could influence the rendering platform used.

@keirthana keirthana requested a review from adglkh November 20, 2024 08:09
Copy link
Contributor

@ajanon ajanon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@adglkh adglkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@morphis morphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall reads much better but we should still get rid of the platform usage as IMHO it might add more confusion/involves another advanced topic the user might not care about to know at this point. Wdyt?

explanation/gpus-instances.md Outdated Show resolved Hide resolved
@adglkh
Copy link
Contributor

adglkh commented Nov 21, 2024

get rid of the platform usage as IMHO it might add more confusion/involves another advanced topic the user might not care about to know at this point. Wdyt?

A: Yeah, sort of. However, if we want to remove the platform context here, we should avoid mentioning software and hardware rendering or video encoding (which are the focus of the chapter about using the GPU in instances). Instead, we should only refer to the visual output, which could be influenced by the --enable-graphics flag. In other word, the --enable-graphics flag doesn't directly determine whether the GPU is used or not; it only decides whether there is visual output from the instance.

@morphis
Copy link
Collaborator

morphis commented Nov 21, 2024

get rid of the platform usage as IMHO it might add more confusion/involves another advanced topic the user might not care about to know at this point. Wdyt?

A: Yeah, sort of. However, if we want to remove the platform context here, we should avoid mentioning software and hardware rendering or video encoding (which are the focus of the chapter about using the GPU in instances). Instead, we should only refer to the visual output, which could be influenced by the --enable-graphics flag. In other word, the --enable-graphics flag doesn't directly determine whether the GPU is used or not; it only decides whether there is visual output from the instance.

We are in a page titled "GPUs and instances" so why shouldn't we talk about hardware rendering and video encoding? Both are essential functionalities we use from a GPU.

We need to explain the --enable-graphics switch as even if you assign a GPU to the instance you won't get it being used by Anbox unless you have it set.

* Change title to reflect current content
* Focus on purpose of `--enable-graphics` flag
(render visual output)
* Fix redirects
@keirthana keirthana requested review from morphis and adglkh November 21, 2024 17:58
@keirthana
Copy link
Collaborator Author

@morphis @adglkh I have rearranged the entire topic to focus on graphical rendering based on the --enable-graphics flag. Please have a look.

I intentionally removed the information about --enable-streaming flag that we had in the earlier round, as it spoke about the rendering platform rather than the purpose of use.

explanation/landing.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
explanation/rendering-graphics.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@morphis morphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@morphis
Copy link
Collaborator

morphis commented Nov 27, 2024

@adglkh do you want to take another look?

Copy link
Contributor

@adglkh adglkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@keirthana keirthana merged commit b9cf08e into canonical:main Nov 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: it makes confusing
4 participants