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

[Shaleen Jain]

On Wed, 2018-12-05 at 09:03 +0100, Thomas Guillem wrote:
> 
> 
> 
> 
> 
> 
> 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.
> 

Okay, done.







-- 


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




> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
https://mailman.videolan.org/listinfo/vlc-devel
-- 
Regards,
Shaleen Jain
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20181205/cb448f75/attachment.html>