VLC Support #83

Closed
wants to merge 99 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

TheLQ commented Aug 26, 2012

I've sat on this for a while trying to figure out any bugs but think its stable enough to be merged into the master repo

Known bugs

  • Some higher quality videos will generate a garbled video output. A temporary fix is to lower the video scale in the GUI. More testing is needed though for an automatic solution
  • Certain combinations of high quality audio and sample rates cause VLC to not generate audio at all or core dump. Once again, more testing is needed before these can be automatically rejected

I think this check is broken for version 2.1.0.
You could use something like this:

import org.apache.maven.artifact.versioning.DefaultArtifactVersion;

private static final String MIN_VLC_VERSION = "2.0.2";

// Compare version number
DefaultArtifactVersion v1 = new DefaultArtifactVersion(MIN_VLC_VERSION);
DefaultArtifactVersion v2 = new DefaultArtifactVersion(registry.getVlcv());

if (v1.compareTo(v2) > 0) {
    LOGGER.error("Only VLC versions " + MIN_VLC_VERSION + " and above are supported");
}

Side note: I was using version 1.0.1 and didn't see any log messages?

Owner

TheLQ replied Jul 15, 2012

I think that only works on Windows, considering that it comes from a registry class. I'm on Linux and I don't even see "Found VideoLAN version X at: Y". What OS were you using?

Also org.apache.maven.artifact.versioning isn't available as a library, and I'm hesitant to add it as a dependency

I'm on OSX.

As for the artifact dependency, isn't that something like this?

<dependency>
    <groupId>org.apache.maven</groupId>
    <artifactId>maven-artifact</artifactId>
    <version>3.0.3</version>
</dependency>

We already have code to parse and compare version strings (though it should probably be moved to e.g. net.pms.util).

TheLQ and others added some commits Jul 15, 2012

Reverted scale default to 1.0. Not every video needs to be downscaled…
…, and medium to low quality vids turn out horrible
If subtitle or language is specified by user going to the #Transcode#…
… folder, acknowledge it. Otherwise, use gui defaults
Moved extra parameters to end of command, might make it so it overrid…
…es options instead of being overwritten by defaults

Out of curiosity, why did you use WMV2 instead of WMV3. Is it because of stability?
Since you are configuring each renderer for VLC, then I would recommend using WMV3 as WMV3 is designed for HD videos and is supported by both the Xbox360 and the PS3.

What does vlc_config add that isn't already covered by TranscodeVideo=WMV?

Owner

TheLQ replied Jul 27, 2012

Up until now its been because I thought VLC didn't support wmv3 ( http://wiki.videolan.org/Codec ). However if you click thorugh the link it says that it has been supported for quite some time.

Of course when I try it in Fedora it seems my repo supplied ffmpeg library wasn't compiled with wmv3 support, so I can't play anything. So for now I'm going to keep it a wmv2. Feel free to test wmv3 with the gui though

Owner

TheLQ replied Jul 28, 2012

It can specify exactly what codecs to use that work on this specific non-standards compliant media renderer. WMV doesn't tell me much as there are 3 wmv video codecs alone.

Also note that the wmv2, wma, asf codec choices were the only permutation that I was able to get working. All others didn't play on the XBox for some reason or caused VLC to crash

We don't want to specify "exactly what codecs to use that work on this specific non-standards compliant media renderer", since there's only one (which also happens to work for the Nokia N-900), and we already have more than enough hacks in place to handle it without granting it an even greater influence over PMS internals. Please don't waste time and effort re-inventing the wheel.

Owner

TheLQ replied Jul 28, 2012

So exactly how do you expect me to detect "Hey, an XBox can't play DVD-Video codecs like every other standards compliant player so I need to use different codecs" without ANY flag in the configuration file?

I'm not re-inventing the wheel, I'm adding a single line of configuration (I'm not entirely sure which wheel I'm inventing)

The flag is TranscodeVideo=WMV. It means "the renderer is an Xbox 360 (or Xbox compatible); use the codecs/container MEncoder currently uses to stream to the Xbox".

Owner

TheLQ replied Jul 28, 2012

So I write an exception for Xbox in code, now what about the other ~26 renderers that have "TranscodeVideo in them (grep the dir yourself)? So now I write an exception for every permutation, then a new renderer comes out, so now I have to update my exceptions? And what about the container? Now I have to match profiles to containers, which is going to suck.

This is the easiest, most efficient, most flexible, and most reliable method to make sure that video's stream to all media renderers without massive headache with only 1 configuration change

I'm confused how 1 line of configuration is worse than lots of fragile auto-detection code and continuous manually managed codec maps that may or may not work.

I'm confused

I'll try to explain it one last time.

There is only one renderer that requires transcoding to WMV - the Xbox (ignore the N-100, since that appears to be confusing you). We indicate this by assigning the WMV profile to the TranscodeVideo option in XBOX360.conf. (All the other renderer confs transcode to MPEG-2.)

Video engines that wish to support the Xbox (most don't), can check if the output should be "WMV" (meaning "whatever codecs/container MEncoder uses for Xbox transcoding") by calling:

if (params.mediaRenderer.isTranscodeToWMV()) { ... }

There are no other renderers that require transcoding to WMV, since no other vendors "embrace and extend" (i.e. break) the DLNA spec in this way.

fragile auto-detection code

There is no auto-detection, fragile or otherwise. The transcode profile is stated explicitly in the renderer conf:

TranscodeVideo=WMV

manually managed codec maps that may or may not work.

A "codec map", as you call it, is more commonly known as a profile. It allows a combination of features (in this case codecs and a container) to be represented by a single identifier. In this case, it also shields the public (i.e. user-facing) API from implementation details that may be subject to change/improvement. It's hardly an uncommon or difficult-to-grasp concept in this sphere:

Now I have to match profiles to containers, which is going to suck.

There are 3 transcode profiles and up to 3 ways to set the transcode format, none of which "suck".

  1. Just output MPEG-2 video and AC-3 audio in an MPEG-PS container (FFmpeg Video and FFmpeg Web Video do this).

  2. Check whether the MPEG-2 output should be PS or TS. Both FFmpeg engines should do this as there are 3 renderers that (claim to) require MPEG-TS, rather than the default MPEG-PS.

  3. Handle the 3 profiles as MEncoder does (adjust to taste):

RendererConfiguration rendererConf = params.mediaRenderer;
String transcode, mux;

if (rendererConf.isTranscodeToWMV()) { // Xbox
    transcode = "acodec=wma2,vcodec=wmv2,channels=2,ab=448";
    mux = "asf";
} else { // MPEG-2
    transcode = "acodec=mp2a,vcodec=mp2v";
    mux = rendererConf.isTranscodeToMPEGTSAC3() ? "ts" : "ps";
}

transcodeSpec = String.format("#transcode{%s}:standard{access=file,mux=%s,dst=...}", transcode, mux);

TheLQ and others added some commits Jul 27, 2012

Set up scaleDefault is not necessary. It could be done in PmsConfiguration as a default value for getVlcScale as I did..

Owner

chocolateboy commented Aug 26, 2012

Thanks for this.

Some nits:

Please follow the style of the existing code:

//comment

should be spaced e.g.:

// comment

Curly braces should be used for conditional statements e.g.:

if (foo)
    bar();

should be:

if (foo) {
    bar();
}

@ghost ghost assigned chocolateboy Aug 26, 2012

Contributor

TheLQ commented Aug 26, 2012

Sorry about that, personal preference. Added in Revision da4efe4

Owner

chocolateboy commented Aug 26, 2012

Just to keep you in the loop: this is a candidate for the 1.80.0 release rather than the forthcoming 1.70.0 release, so you have plenty of time to tweak.

One suggestion (not a prerequisite for merging): have you had a chance to look at the VLC web video and VLC web audio engines? It would be nice to update them to reflect these changes. This will happen anyway, eventually, but a consideration of functionality that is common to all four engines (we'll add a VLC audio engine as well) might point to improvements in this engine (e.g. by abstracting common code).

+ private static final String KEY_VLC_USE_HW_ACCELERATION = "VLC_use_HW_acceleration";
+ private static final String KEY_VLC_USE_EXPERIMENTAL_CODECS = "VLC_use_experimental_codecs";
+ private static final String KEY_VLC_AUDIO_SYNC_ENABLED = "VLC_audio_sync_enabled";
+ private static final String KEY_VLC_SUBTITLE_ENABLED = "VLC_subtitle_enabled";
@chocolateboy

chocolateboy Aug 26, 2012

Owner

There's already a global toggle for subtitles. No need to enable/disable on a per-engine basis.

@TheLQ

TheLQ Aug 29, 2012

Contributor

Addressed in Rev 86d14f2. However I would like to say a) isMencoderDisableSubtitles implies mencoder only and b) even though the checkbox is in the Transcoding tab, the event handler is defined in the Mencoder class. This means that when the mencoder engine isn't loaded (had some issues with the executable at first), the configuration option is broken.

@chocolateboy

chocolateboy Sep 12, 2012

Owner

FYI: this is being addressed in a separate pull request.

+ private static final String KEY_VLC_AUDIO_SYNC_ENABLED = "VLC_audio_sync_enabled";
+ private static final String KEY_VLC_SUBTITLE_ENABLED = "VLC_subtitle_enabled";
+ private static final String KEY_VLC_AUDIO_PRI = "VLC_audio_language_priority";
+ private static final String KEY_VLC_SUBTITLE_PRI = "VLC_subtitle_language_priority";
@chocolateboy

chocolateboy Aug 26, 2012

Owner

L183-184: I understand why you've done this (because MEncoder did it), but the audio and subtitle language priorities should really be global (i.e. on the "Common transcode settings" pane) rather than per-engine.

+ private static final String KEY_VLC_CODEC_OVERRIDE = "VLC_codec_override";
+ private static final String KEY_VLC_SCALE = "VLC_scale";
+ private static final String KEY_VLC_SAMPLE_RATE_OVERRIDE = "VLC_sample_rate_override";
+ private static final String KEY_VLC_SAMPLE_RATE = "VLC_sample_rate";
@chocolateboy

chocolateboy Aug 26, 2012

Owner

L185-191: I need to see these in the GUI, but in principle a case needs to be made for each one. The MEncoder engine is deprecated and will be removed. As a general rule, "MEncoder did it" is a reason to _not_ do the same in a new engine.

_Edit_. checked it in the GUI.

Please rename "Extra parameters" -> "Custom options". It's also weirdly indented (as is "Video scale").

What's the rationale for video scale?

I'm not convinced by the custom acodec/vcodec/container settings. The only pro is that it allows users to experiment with new profiles. The cons are that "experiment with new profiles" is not a real use case (developers can easily do that without these options); it's a MEncoder-ish hack (global rather than per-renderer) in an engine that we're trying to position as the sane MEncoder replacement (and making it available at the renderer level is another MEncoder hack we don't want to repeat); it also clutters the UI, and may complicate troubleshooting issues reported by users who (as often happens) set it and forget it.

For now, please remove it. If a compelling use case is demonstrated, it can easily be reinstated.

What's the rationale for the audio sample rate options? Why is there a checkbox and a value rather than just a (possibly blank) value? Again, it also looks very cluttered.

I'm not sure how to do this, but ideally the whole pane should be grayed out (disabled) if the engine is not installed/compatible.

@TheLQ

TheLQ Aug 29, 2012

Contributor

Renamed Extra Parameters in Rev c796504, makes more sense.

Those options were really useful during development, especially when I tried to write the fancy auto-detection code for the first time. I honestly think there should be a "developer mode" where it would be shown as from personal experience changing a codec, compiling, testing, changing again... is a really long tedious process. For now though, its removed in Rev 584ec39

Audio sample rate was added to debug VLC crashes. Considering that there is a remote chance that a different sample rate will completely fix the audio codec crashing I believe it should be in the GUI. For the checkbox and input field setup, I thought it could be useful if you didn't want it enabled all the time but had the magical sample rate that fixed the bug. If it still bothers you I guess I could make it a single text box with an empty check

Lastly, doesn't PMS handle not displaying the GUI if the engine isn't loaded correctly?

+ config.audioCodec = "mp2a"; // NOTE: a52 sometimes causes audio to stop after ~5 mins
+ config.container = type;
+ } else {
+ throw new RuntimeException("Unknown encoding profile");
@chocolateboy

chocolateboy Aug 26, 2012

Owner

Check for WMV, then MPEGTSAC3; if it's neither of those, use the default profile: MPEGPSAC3. There's nothing wrong with validating the value, but, if needed, it should be done in RendererConfiguration, not here.

+ return config;
+ }
+
+ public class CodecConfig {
@chocolateboy

chocolateboy Aug 26, 2012

Owner

Why is this public?

@TheLQ

TheLQ Aug 29, 2012

Contributor

Made sense at the time I guess. Addressed in Rev 52d7121

+ PipeProcess tsPipe = new PipeProcess("VLC" + System.currentTimeMillis() + "." + config.container);
+ ProcessWrapper pipe_process = tsPipe.getPipeProcess();
+
+ LOGGER.debug("filename: " + fileName);
@chocolateboy

chocolateboy Aug 26, 2012

Owner

For release, almost all of these messages should be logged at trace level rather than debug.

@TheLQ

TheLQ Aug 29, 2012

Contributor

Addressed in Rev bb023c5.

+ }
+ });
+
+ // Developer stuff. Theoretically is temporary
@chocolateboy

chocolateboy Aug 26, 2012

Owner

Time to remove the temporary developer stuff :-)

@TheLQ

TheLQ Aug 29, 2012

Contributor

Probably should of said "Advanced options". As said in other comments, some of the options are useful.

+
+ //Break down version number
+ String[] vlcVersion = registry.getVlcv().split("\\.", 3);
+ if(Integer.parseInt(vlcVersion[0]) < 2 || Integer.parseInt(vlcVersion[2]) < 2) {
@chocolateboy

chocolateboy Aug 26, 2012

Owner

Space after if.

@@ -403,6 +403,12 @@ public void configurationChanged(ConfigurationEvent event) {
if (registry.getVlcv() != null && registry.getVlcp() != null) {
@chocolateboy

chocolateboy Aug 26, 2012

Owner

As you mentioned, this only works on Windows. It also fails if the path is set via vlc_path.

It should be possible to probe the version via vlc -I dummy [ --dummy-quiet ] --version. Unfortunately, we don't have a straightforward way to run simple one-off commands like this (I'm looking into it).

@TheLQ

TheLQ Aug 29, 2012

Contributor

There are several examples online (IE http://stackoverflow.com/questions/3643939/java-process-with-input-output-stream ) of different ways to read all a processes input. I think though someone more familiar with PMS internals should write it as I don't know if there's any PMS specific classes that you would want to use

@chocolateboy

chocolateboy Aug 31, 2012

Owner

I think though someone more familiar with PMS internals should write it

Like I said, I'm working on it.

@@ -301,3 +301,24 @@ TrTab2.65=Low quality, low-end CPU or HD Wifi transcoding
TracesTab.3=Clear
TranscodeVirtualFolder.0=\#--TRANSCODE--\#
TreeNodeSettings.4=This engine is not loaded\!
+VlcTrans.1=VLC Transcoder Settings
@chocolateboy

chocolateboy Aug 26, 2012

Owner

Please use the VLCVideo "namespace" for these.

src/main/java/net/pms/formats/MPG.java
@@ -44,7 +44,9 @@ public Identifier getIdentifier() {
ArrayList<Class<? extends Player>> a = new ArrayList<Class<? extends Player>>();
PMS r3 = PMS.get();
for (String engine : PMS.getConfiguration().getEnginesAsList(r3.getRegistry())) {
- if (engine.equals(MEncoderVideo.ID)) {
+ if(engine.equals(VLCVideo.ID))
@chocolateboy

chocolateboy Aug 26, 2012

Owner

Space after if and curly brackets.

+ */
+ protected CodecConfig genConfig(RendererConfiguration renderer) {
+ CodecConfig config = new CodecConfig();
+ if (codecOverride.isSelected()) {
@chocolateboy

chocolateboy Aug 27, 2012

Owner

Please use the PmsConfiguration getters rather than probing the GUI. For starters, there is no guarantee that there is a GUI (headless servers). It's also a good way to verify the setters are working and being called.

Owner

TheLQ commented on 584ec39 Aug 29, 2012

Opps, switched OS's and forgot to update my git config. Since its already pushed out I think the damage of trying to fix it isn't worth it

Member

Raptor399 commented Mar 29, 2013

Pull request merged in commit e3f4d4e.
Thanks, @lordquackstar !

@Raptor399 Raptor399 closed this Mar 29, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment