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

Encoding profile option #1226

Closed

Conversation

tzah4748
Copy link

There are some decoders like Broadway that can accept only H.264 Baseline profile streams.
This profile is considered the most generic and is available on most Android devices.
To allow selecting a profile new option has been added to client and server.

Tzah Mazuz added 6 commits March 19, 2020 17:31
If the option is not requested with -c then the codec will be automatically choosen by the MediaCodec.

(cherry picked from commit 43aea79)
(cherry picked from commit d4fb6a0)
(cherry picked from commit e7f205c)
(cherry picked from commit ef5d72c)
(cherry picked from commit 7e36540)
(cherry picked from commit ef95046)
(cherry picked from commit fa3644c)
(cherry picked from commit e158243)
(cherry picked from commit a4ef318)
(cherry picked from commit a5b83c1)
app/src/cli.c Show resolved Hide resolved
@@ -431,6 +466,7 @@ scrcpy_parse_args(struct scrcpy_cli_args *args, int argc, char *argv[]) {
{"window-height", required_argument, NULL, OPT_WINDOW_HEIGHT},
{"window-borderless", no_argument, NULL,
OPT_WINDOW_BORDERLESS},
{"codec-profile", required_argument, NULL, 'P'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an advanced option, I would prefer not to consume a "short" option (-P) for that.

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's totally fine, at first i had a bit of a problem understanding the code so i didn't know if it will work without a short version of the option, its ok to remove it -P and stay with --codec-profile only

@tzah4748
Copy link
Author

tzah4748 commented Mar 19, 2020 via email

@rom1v
Copy link
Collaborator

rom1v commented Mar 19, 2020

What if you don't set the level value at all on the MediaFormat (only the profile)?

@tzah4748
Copy link
Author

tzah4748 commented Mar 20, 2020 via email

@rom1v
Copy link
Collaborator

rom1v commented Mar 20, 2020

If the level is chosen automatically when not set, then maybe an option --codec-profile accepting any integer and a --codec-level accepting any integer (ignored if API < 23) would be better?

@tzah4748
Copy link
Author

tzah4748 commented Mar 20, 2020 via email

@rom1v
Copy link
Collaborator

rom1v commented Mar 20, 2020

Oh, maybe we could do it very generically: instead of adding codec options when people need them, we could just pass a string of key=value.

For example:

scrcpy --codec-options profile=1,level=2

The String would be passed as-is from the client to the server, and the server would parse it and set the media options.

There is an additional detail: the type depends on the key. Some expect an integer value, some others a long, some others a string.

Since almost all are integers, I suggest to use integer by default.
Otherwise, we could specify the type:

--codec-options somekey:string=value

What do you think?

@tzah4748
Copy link
Author

tzah4748 commented Mar 20, 2020 via email

@rom1v
Copy link
Collaborator

rom1v commented Mar 20, 2020

And what if a person doesn't want to specify a level ? Just the profile?

Yes, the idea is to pass anything you want from MediaFormat.

scrcpy --codec-options key1:value1,key2=value2,...,keyn=valuen

Scrcpy would not even "understand" that it's a profile or a level, it just sets the value to the MediaFormat instance.

@tzah4748
Copy link
Author

tzah4748 commented Mar 20, 2020 via email

@rom1v
Copy link
Collaborator

rom1v commented Mar 20, 2020

I don't think i understood what you meant

Instead of --codec-level, --codec-profile, --codec-something options, we could have only one option: --codec-options passing all key/values as a single string.

That way, one could pass any present and future codec options as they need.

@rom1v
Copy link
Collaborator

rom1v commented Mar 21, 2020

@tzah4748 Are you ok for implementing such a --codec-options parameter instead of a specific option --codec-profile?

@tzah4748
Copy link
Author

tzah4748 commented Mar 22, 2020 via email

@tzah4748
Copy link
Author

@rom1v Opened a new PR with what we talked about.

@tzah4748 tzah4748 closed this Apr 17, 2020
@rom1v rom1v mentioned this pull request Apr 18, 2020
@rom1v
Copy link
Collaborator

rom1v commented May 3, 2020

Reopen (see #1325 (review))

@rom1v rom1v reopened this May 3, 2020
@rom1v
Copy link
Collaborator

rom1v commented May 5, 2020

On my device (after fixing compilation errors), it does not work as is (of course, it works without --codec-profile):

$ scrcpy --codec-profile 2
[server] ERROR: Exception on thread Thread[main,5,main]
android.media.MediaCodec$CodecException: Error 0xfffffc0e
	at android.media.MediaCodec.native_configure(Native Method)
	at android.media.MediaCodec.configure(MediaCodec.java:1778)
	at com.genymobile.scrcpy.ScreenEncoder.configure(ScreenEncoder.java:191)
	at com.genymobile.scrcpy.ScreenEncoder.streamScreen(ScreenEncoder.java:76)
	at com.genymobile.scrcpy.Server.scrcpy(Server.java:35)
	at com.genymobile.scrcpy.Server.main(Server.java:170)
	at com.android.internal.os.RuntimeInit.nativeFinishInit(Native Method)
	at com.android.internal.os.RuntimeInit.main(RuntimeInit.java:251)

Computed max level is 65536 (0x10000, AVCLevel52). If I programmatically set level to 4096 (0x1000, AVCLevel41), it works.

Therefore, I think using the max available level for the given profile is not a good strategy 😕

@rom1v
Copy link
Collaborator

rom1v commented May 5, 2020

Oh I guess it requires changes from #1296?

@tzah4748
Copy link
Author

tzah4748 commented May 6, 2020

@rom1v Yes Rom, in #1296 I've added logic to choose the right level depending on the device screen size.
It should depend on the bit-rate as well but i ignored it for now (since i wasn't sure if its really needed) and it worked just fine.
In short, it has suggested level logic

@rom1v
Copy link
Collaborator

rom1v commented May 6, 2020

OK, thank you for your answer. 👍

If selecting a correct level based on the profile was straightforward, I would be in favor on doing it automatically.

However, it seems complicated, so it would add complex code to maintain, and to modify when scrcpy will support other codecs, for a very advanced option. Therefore, I think I prefer that the user provide both profile and level (like #1325).

@tzah4748
Copy link
Author

@rom1v are you gonna do it yourself or you are waiting a new pull request?

@rom1v
Copy link
Collaborator

rom1v commented May 12, 2020

are you gonna do it yourself or you are waiting a new pull request?

I have a working branch (codec_options), I might merge it before the next release.

@tzah4748
Copy link
Author

@rom1v That's great news!
Do you maybe have an ETA when is the next release?

rom1v added a commit that referenced this pull request May 24, 2020
Add a command-line parameter to pass custom options to the device
encoder (as a comma-separated list of "key[:type]=value").

The list of possible codec options is available in the Android
documentation:
<https://d.android.com/reference/android/media/MediaFormat>

PR #1325 <#1325>
Refs #1226 <#1226>

Co-authored-by: Romain Vimont <rom@rom1v.com>
@rom1v
Copy link
Collaborator

rom1v commented May 24, 2020

Merged #1325 into dev.

@rom1v rom1v closed this May 24, 2020
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.

2 participants