[vlc-devel] [PATCH 1/3] chromecast: refactor out encoder option functions

Thomas Guillem thomas at gllm.fr
Wed Dec 5 09:03:32 CET 2018




On Tue, Dec 4, 2018, at 10:15, Shaleen Jain wrote:
> Hi,
> 
> On Tue, 2018-12-04 at 09:03 +0100, Thomas Guillem wrote:
>> 
>> 
>> 
>> On Mon, Dec 3, 2018, at 14:31, Shaleen Jain wrote:
>>> On Mon, 2018-12-03 at 12:10 +0100, Thomas Guillem wrote:
>>>> Hello,>>>> 
>>>> 
>>>>
>>>> Could you also refactor renderer module options, like "show-perf-
>>>> warning", "audio-passthrough", "conversion-quality", and maybe some
>>>> others.>>>>
>>>> You can check how it's done in
>>>> modules/video_output/opengl/display.c, with add_glopts().>>>> 
>>>> 
>>>> On Wed, Nov 28, 2018, at 16:44, Shaleen Jain wrote:
>>> 
>>> Do you mean to use a define for the common options? besides the
>>> three that you have already mentioned "ip" and "port" are the common>>> options as they are required by vlc_renderer_item_new but they have
>>> different defaults for chromecast and dlna each (for e.g chromecast
>>> has port 8009 and dlna has port 7070).>>> So I'm not sure if it'll be wise to make them (ip and port) common
>>> for each renderer.>> 
>> You're right, it won't.
>> Each modules should have their own options name:
>> "chromecast-something", "upnp-renderer-something" and not a common
>> "renderer-something". This will allow users to setup different
>> options for chromecast and upnp.>> 
>> That said, modules can have different options name but using common
>> options via some includes.>> For ip and port, you could specifiy the default as a MACRO parameter.> 
> I think that would unnecessarily complicate things. 
> ip and port and other options are options that would only be setup by
> another module (the module acting as the renderer_discovery)> and no user will change it manually so it would be fine to let the
> sout module add defines for them with the default and the module
> specific prefix.
OK.

> 
> But for the common options that would be exposed to the user and
> influenced by the platform like "show-perf-warning", "audio-
> passthrough", "conversion-quality", I have them as a single prefix ("sout-renderer-
> ") and the marco "add_renderer_opts"
The "conversion-quality" setting must be chosen according to the
chromecast device performance and according to the quality of the
network. Therefore, I don't like sharing the same option string
that the upnp one (The device handling upnp can have better perfs
that the chromecast, so you can choose higher perfs for the upnp
case for example).
Same for "audio-passthrought".

"show-perf-warning" can be common to all modules.


> 
> Please take a look at this patch here:
> https://mailman.videolan.org/pipermail/vlc-devel/2018-December/122042.html> it implements the things I have described above.

OK, I like that. But like I said I would have added a suffix parameter
for "audio-passthrough" and "conversion-quality" (chromecast or upnp).
PS:You didn't added copyright headers and vlc suffixes to renderer functions like I said in the fist review.
> 
>> 
>>> 
>>> 
>>> 
>>> -- 
>>> 
>>> Regards,
>>> Shaleen Jain
>>> 
>>> _________________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>>
>> _______________________________________________
>> vlc-devel mailing list To unsubscribe or modify your subscription
>> options:>> 
>> https://mailman.videolan.org/listinfo/vlc-devel
>> 
> 
> -- 
> Regards,
> Shaleen Jain
> 
> _________________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20181205/9637bb4e/attachment.html>


More information about the vlc-devel mailing list