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.

Sign up for GitHub

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

Jump to bottom

Encoding options #1296

Closed
wants to merge 9 commits into from
Closed

Encoding options #1296

wants to merge 9 commits into from

Conversation

tzah4748
Copy link

Introduced options arguments for the Android MediaCodec library.

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 7 commits April 14, 2020 18:26
Adding codec_profile to scrcpy client default options.
8f9205e 
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 504d6a4)
Adding codecProfile to server options
88a00ab 
(cherry picked from commit d4fb6a0)
(cherry picked from commit e7f205c)
(cherry picked from commit ef5d72c)
(cherry picked from commit 709a0c3)
Adding codecProfile and setCodecProfile to the ScreenEncoder
15c0d5e 
(cherry picked from commit 2505242)
(cherry picked from commit a9685d3)
Fix the printed versions (were opposite)
fd5489d 
(cherry picked from commit 7e36540)
(cherry picked from commit ef95046)
(cherry picked from commit fa3644c)
(cherry picked from commit a5d5ea1)
Added help and parsing of the codec-profile option
bc70f56 
(cherry picked from commit e158243)
(cherry picked from commit a4ef318)
(cherry picked from commit a5b83c1)
(cherry picked from commit 1e3321c)
Adding codec-profile option to the server exec params
eaef1c1 
(cherry picked from commit fa56950)
(cherry picked from commit c12306a6c2c15c9368feab25f298e97015ee07eb)
Changing codec-profile -> codec-options
c265d63
rom1v
rom1v reviewed Apr 18, 2020
View reviewed changes
Copy link
Collaborator

@rom1v rom1v left a comment

Choose a reason for hiding this comment

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

My idea in #1226 (comment) was to support any key, not a predefined set of keys.

In other words, scrcpy would just delegate the key and the value to:

format.setInteger(key, value);

(in the case of integers)

Without any "understanding" of what the key or the value is.

Sorry, I probably should have been more explicit :/

What do you think of just forwarding key/value pairs?

(Also, please update your dev branch)

server/src/main/java/com/genymobile/scrcpy/CodecOptions.java
import java.util.LinkedHashMap;
import java.util.Map;

public class CodecOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(so this class would not be needed at all)

Copy link
Author

Choose a reason for hiding this comment

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

I don't think its a smart idea to let the user pass down any key=value pair he wants...
He can totally mess up and give random values and things that might even break the encoding and he won't be aware that he was giving something wrong.

I don't know about the other options but in case of profile and level it is suggested that if you provide a profile, you will also set a level, this is why when only profile (without level) is provided by the user, there is a calculation based on the screen size that should decide the level.

My goal was not to allow the user to pass down any key=value pair he wants, but to allow you or whoever wants to, to expand on the current CodecOptions and easily add new options that he can choose how they will be parsed and what will happen with them and where in the code.

Copy link
Collaborator

@rom1v rom1v Apr 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think its a smart idea to let the user pass down any key=value pair he wants...

It would allow to configure the codec generically, without having to build the app.

For advanced options, I would prefer a generic way.

He can totally mess up and give random values and things that might even break the encoding and he won't be aware that he was giving something wrong.

At least, we can warn if the provided key is not supported: containsKey(). (EDIT: in fact, it just tells whether we set a key for the format)

User can already pass random values (a bitrate of 1Mbps for instance), which is "ignored" by the codec without warnings.

I don't know about the other options but in case of profile and level it is suggested that if you provide a profile, you will also set a level, this is why when only profile (without level) is provided by the user, there is a calculation based on the screen size that should decide the level.

Isn't it done automatically if you don't provide the level? #1226 (comment)

Btw, contrary to what I said here, there is no need to specify the type in the string, it can be requested: getValueTypeForKey

Copy link
Author

Choose a reason for hiding this comment

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

OK, it will be done your way :)
Thanks for the good lesson.

Copy link
Author

Choose a reason for hiding this comment

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

@rom1v
Just a quick question, what if the key is supported but value is not supported by the device?
If we are assuming the codec ignores bad values shouldn't the user know about it somehow?

Copy link
Author

Choose a reason for hiding this comment

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

getValueTypeForKey is API29+ ...
What do you suggest?
We can go back to what you said, assuming int unless specified key:type=value

Copy link
Author

Choose a reason for hiding this comment

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

@rom1v Ping

Copy link
Collaborator

Choose a reason for hiding this comment

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

what if the key is supported but value is not supported by the device?

The device will just ignore it I guess. Like if for now you pass --max-fps but the encoder does not support it.

getValueTypeForKey is API29+

:(

We can go back to what you said, assuming int unless specified key:type=value

Hmm, yes... I have no better solution then :(

Copy link
Author

@tzah4748 tzah4748 Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK, so I've made the changes on our side, ill need to update my dev branch and make a new PR.
@rom1v
Just to make things clear before i make a new PR, the expected value types are
Integer/int - String/str - Float/float - Long/long
Yes?

@rom1v
Copy link
Collaborator

rom1v commented Apr 18, 2020

(I applied this quick-change from your PR: 125c556)

@tzah4748 tzah4748 closed this Apr 26, 2020
@tzah4748 tzah4748 deleted the encoding-options branch April 26, 2020 12:15
@rom1v rom1v mentioned this pull request May 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rom1v rom1v rom1v left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tzah4748 @rom1v