Bad hack breaks contentFeatures when transcoding to non-NTSC

[Nadahar]

Based on this forum thread. Although I can't say for sure that this is the cause for the user's problem, I can say for sure that this is a terrible hack that will break a lot. It simply seems like somebody didn't finish their code and it has stayed this way.

When looking at one of the problematic files, this is the DLNA information given in the DIDL-Lite:

DLNA.ORG_PN=MPEG_PS_PAL;DLNA.ORG_OP=01

Later when the renderer asks for contentFeatures for the same video, this is sent:

DLNA.ORG_PN=MPEG_PS_NTSC;DLNA.ORG_OP=01;DLNA.ORG_CI=0;DLNA.ORG_FLAGS=01700000000000000000000000000000

According to DLNA these are required to be identical. I suspect that CI and FLAGS doesn't cause much havoc though, but the PN definitly will.

The reason is also easy to see:

UniversalMediaServer/src/main/java/net/pms/dlna/DLNAResource.java

Lines 442 to 447 in 5d1a197

	public String getDlnaContentFeatures(RendererConfiguration mediaRenderer) {
	// TODO: Determine renderer's correct localization value
	int localizationValue = 1;
	String dlnaOrgPnFlags = getDlnaOrgPnFlags(mediaRenderer, localizationValue);
	return (dlnaOrgPnFlags != null ? (dlnaOrgPnFlags + ";") : "") + getDlnaOrgOpFlags(mediaRenderer) + ";DLNA.ORG_CI=0;DLNA.ORG_FLAGS=01700000000000000000000000000000";
	}

localizationValue == 1 simply means NTSC, so NTSC will always be used for contentFeatures. Although there's a comment there, that doesn't help. The code is still broken. I don't see any obvious way to know what value to use either, since UMS doesn't track what <res> element the renderer picks. From what I can see from the very messy code generating the DIDL, one version is offered for each "localization" if the renderer "require localization". That isn't the case for the renderer in question here, so only one is returned. How that ends up being PAL is beyond me, but it's probably due to some other hack. The code seems to me to simply offer NTSC to any renderer that doesn't "require localization".

This will fail also if the renderer is set to "require localization" because the renderer will then be offered different versions of the file (or at least with different DLNA profiles while actual compliance to the particular profile is ignored). If the renderer doesn't pick the first (NTSC/NA version), and later requests contentFeatures the wrong result will be returned again - since it's hardcoded to 1.

I have my hands on too many other things right now so I won't solve this. I think this is a very bad bug, and I urge some of you to fix it. Despite the fact that contentFeatures is somewhat "deprecated" by DLNA, it seems that many relatively new renderers request this. It is a mandatory function in any case according to DLNA, and the information is required to be absolutely identical in DIDL-Lite and contentFeatures - which means that renderer developers rely on this to be true.

For reference, this is the mess of a code that "handles" the "localization":

UniversalMediaServer/src/main/java/net/pms/dlna/DLNAResource.java

Lines 2494 to 2666 in 5d1a197

	int indexCount = 1;
	if (mediaRenderer.isDLNALocalizationRequired()) {
	indexCount = getDLNALocalesCount();
	}
	for (int c = 0; c < indexCount; c++) {
	openTag(sb, "res");
	addAttribute(sb, "xmlns:dlna", "urn:schemas-dlna-org:metadata-1-0/");
	String dlnaOrgPnFlags = getDlnaOrgPnFlags(mediaRenderer, c);
	String tempString = "http-get:*:" + getRendererMimeType(mediaRenderer) + ":" + (dlnaOrgPnFlags != null ? (dlnaOrgPnFlags + ";") : "") + getDlnaOrgOpFlags(mediaRenderer);
	wireshark.append(' ').append(tempString);
	addAttribute(sb, "protocolInfo", tempString);
	if (subsAreValidForStreaming && mediaRenderer.offerSubtitlesByProtocolInfo() && !mediaRenderer.useClosedCaption()) {
	addAttribute(sb, "pv:subtitleFileType", media_subtitle.getType().getExtension().toUpperCase());
	wireshark.append(" pv:subtitleFileType=").append(media_subtitle.getType().getExtension().toUpperCase());
	addAttribute(sb, "pv:subtitleFileUri", getSubsURL(media_subtitle));
	wireshark.append(" pv:subtitleFileUri=").append(getSubsURL(media_subtitle));
	}
	if (getFormat() != null && getFormat().isVideo() && media != null && media.isMediaparsed()) {
	if (player == null) {
	wireshark.append(" size=").append(media.getSize());
	addAttribute(sb, "size", media.getSize());
	} else {
	long transcoded_size = mediaRenderer.getTranscodedSize();
	if (transcoded_size != 0) {
	wireshark.append(" size=").append(transcoded_size);
	addAttribute(sb, "size", transcoded_size);
	}
	}
	if (media.getDuration() != null) {
	if (getSplitRange().isEndLimitAvailable()) {
	wireshark.append(" duration=").append(StringUtil.formatDLNADuration(getSplitRange().getDuration()));
	addAttribute(sb, "duration", StringUtil.formatDLNADuration(getSplitRange().getDuration()));
	} else {
	wireshark.append(" duration=").append(media.getDurationString());
	addAttribute(sb, "duration", media.getDurationString());
	}
	}
	if (media.getResolution() != null) {
	if (player != null && mediaRenderer.isKeepAspectRatio()) {
	addAttribute(sb, "resolution", getResolutionForKeepAR(media.getWidth(), media.getHeight()));
	} else {
	addAttribute(sb, "resolution", media.getResolution());
	}
	}
	addAttribute(sb, "bitrate", media.getRealVideoBitrate());
	if (firstAudioTrack != null) {
	if (firstAudioTrack.getAudioProperties().getNumberOfChannels() > 0) {
	addAttribute(sb, "nrAudioChannels", firstAudioTrack.getAudioProperties().getNumberOfChannels());
	}
	if (firstAudioTrack.getSampleFrequency() != null) {
	addAttribute(sb, "sampleFrequency", firstAudioTrack.getSampleFrequency());
	}
	}
	} else if (getFormat() != null && getFormat().isImage()) {
	if (media != null && media.isMediaparsed()) {
	wireshark.append(" size=").append(media.getSize());
	addAttribute(sb, "size", media.getSize());
	if (media.getResolution() != null) {
	addAttribute(sb, "resolution", media.getResolution());
	}
	} else {
	wireshark.append(" size=").append(length());
	addAttribute(sb, "size", length());
	}
	} else if (getFormat() != null && getFormat().isAudio()) {
	if (media != null && media.isMediaparsed()) {
	if (media.getBitrate() > 0) {
	addAttribute(sb, "bitrate", media.getBitrate());
	}
	if (media.getDuration() != null && media.getDuration().doubleValue() != 0.0) {
	wireshark.append(" duration=").append(StringUtil.formatDLNADuration(media.getDuration()));
	addAttribute(sb, "duration", StringUtil.formatDLNADuration(media.getDuration()));
	}
	int transcodeFrequency = -1;
	int transcodeNumberOfChannels = -1;
	if (firstAudioTrack != null) {
	if (player == null) {
	if (firstAudioTrack.getSampleFrequency() != null) {
	addAttribute(sb, "sampleFrequency", firstAudioTrack.getSampleFrequency());
	}
	if (firstAudioTrack.getAudioProperties().getNumberOfChannels() > 0) {
	addAttribute(sb, "nrAudioChannels", firstAudioTrack.getAudioProperties().getNumberOfChannels());
	}
	} else {
	if (configurationSpecificToRenderer.isAudioResample()) {
	transcodeFrequency = mediaRenderer.isTranscodeAudioTo441() ? 44100 : 48000;
	transcodeNumberOfChannels = 2;
	} else {
	transcodeFrequency = firstAudioTrack.getSampleRate();
	transcodeNumberOfChannels = firstAudioTrack.getAudioProperties().getNumberOfChannels();
	}
	if (transcodeFrequency > 0) {
	addAttribute(sb, "sampleFrequency", transcodeFrequency);
	}
	if (transcodeNumberOfChannels > 0) {
	addAttribute(sb, "nrAudioChannels", transcodeNumberOfChannels);
	}
	}
	}
	if (player == null) {
	if (media.getSize() != 0) {
	wireshark.append(" size=").append(media.getSize());
	addAttribute(sb, "size", media.getSize());
	}
	} else {
	// Calculate WAV size
	if (
	firstAudioTrack != null &&
	media.getDurationInSeconds() > 0.0 &&
	transcodeFrequency > 0 &&
	transcodeNumberOfChannels > 0
	) {
	int finalSize = (int) (media.getDurationInSeconds() * transcodeFrequency * 2 * transcodeNumberOfChannels);
	LOGGER.trace("Calculated transcoded size for {}: {}", getSystemName(), finalSize);
	wireshark.append(" size=").append(finalSize);
	addAttribute(sb, "size", finalSize);
	} else if (media.getSize() > 0){
	LOGGER.trace("Could not calculate transcoded size for {}, using file size: {}", getSystemName(), media.getSize());
	wireshark.append(" size=").append(media.getSize());
	addAttribute(sb, "size", media.getSize());
	}
	}
	} else {
	wireshark.append(" size=").append(length());
	addAttribute(sb, "size", length());
	}
	} else {
	wireshark.append(" size=").append(DLNAMediaInfo.TRANS_SIZE).append(" duration=09:59:59");
	addAttribute(sb, "size", DLNAMediaInfo.TRANS_SIZE);
	addAttribute(sb, "duration", "09:59:59");
	addAttribute(sb, "bitrate", "1000000");
	}
	endTag(sb);
	// Add transcoded format extension to the output stream URL.
	String transcodedExtension = "";
	if (player != null && media != null) {
	// Note: Can't use instanceof below because the audio classes inherit the corresponding video class
	if (media.isVideo()) {
	if (mediaRenderer.isTranscodeToMPEGTS()) {
	transcodedExtension = "_transcoded_to.ts";
	} else if (mediaRenderer.isTranscodeToWMV() && !xbox360) {
	transcodedExtension = "_transcoded_to.wmv";
	} else {
	transcodedExtension = "_transcoded_to.mpg";
	}
	} else if (media.isAudio()) {
	if (mediaRenderer.isTranscodeToMP3()) {
	transcodedExtension = "_transcoded_to.mp3";
	} else if (mediaRenderer.isTranscodeToWAV()) {
	transcodedExtension = "_transcoded_to.wav";
	} else {
	transcodedExtension = "_transcoded_to.pcm";
	}
	}
	}
	wireshark.append(' ').append(getFileURL()).append(transcodedExtension);
	sb.append(getFileURL()).append(transcodedExtension);
	LOGGER.trace("Network debugger: " + wireshark.toString());
	wireshark.setLength(0);
	closeTag(sb, "res");
	}
	}

[Nadahar]

I understand now that this is a feature, not a bug.

[SubJunk]

This is still a bug

[Nadahar]

@SubJunk What I mean by "feature" in this context is that it's the way the code is written. There's no "bug" to fix here, a complete redesign of everything related to protocolInfo and other DLNA metadata communication is needed to be able to fix it.

As I've seen no will to or interest in this for several years there's no other conclusion than that you're happy with the current implementation. The fact that you keep "tweaking" the existing, deeply flawed, implementation also indicates that there's no intent to fix it.

[SubJunk]

It's an incorrect assumption that I'm happy with the current implementation; if I had more time I would work on all of these open issues and ideally get us to really nice, thread-safe, standards-compliant code. However, I am a full time single father, and I work full time.

Other people don't have the same limitations on time and I am focusing on getting more devs onto this project so our issue backlog can make progress

[Nadahar]

This isn't exactly a new situation, it's been like this for years and years, and despite the lack of time I haven't seen that any of the time that is spent on this project is spent on things like that.

I don't doubt that everybody wish the implementation was better though, what I question is the will to make it so.

In the end, do like you want, if you want open issues that will just stay untouched forever, please go ahead. I simply closed it because I no longer have any plan to try to do this, and my impression this far has been that I've been the only one actually trying to do anything about it.

[SubJunk]

Yes you have been the only one so far, but that's not necessarily true of the future, especially since this is the only time since UMS started that I have actively recruited more developers. It would be good to attract another Java-specific developer who can improve thread-safety because I think you were the only one with deep enough knowledge about that. I would need to do a lot of reading in order to feel confident with that, which I am interested in doing if I ever get the time, but that isn't really my job as the project leader.

Ideally I see my role in this project continuing to be as a lead/admin who can facilitate other developers rather than doing the big development myself, since that will allow me to use my time more efficiently. I have failed in that role at certain times because I didn't respond to people quickly enough and it resulted in stale code, but I have been on top of that lately and will continue to focus my efforts on making sure that I do everything I can do get code submissions progressed.

[Nadahar]

It would be very nice if you found some others that were interested in and able to improve the quality of the code (that's what I call these things in short). Regarding your role, that's never been my "problem". What I have found frustrating is that trying to fix these things have only met opposition or being ignored. From everybody. I concluded back in 2016 that this simply isn't a priority, which is why I made DMS to be able to focus on this without having to constantly "fight" for it or have it crapped all over by new code that just makes things worse committed constantly.

You've mentioned several times my "Java knowledge". Just for the record, I don't see myself as being that knowledgeable with Java. I constantly have to do "research" when I try to find the best way to do things, and I had never seen a line of Java in my life when I joined this project. I have picked up some things along the way though, but I don't consider myself anywhere close to the people that are truly knowledgeable about Java.

When it comes to thread safety I've never understood why the rest of you haven't been willing to take this into consideration. It's like it's super difficult or something, which isn't true at all. Thread safety is extremely simple as a concept: Don't let a thread write to something in shared memory that theoretically can be either read or written by another thread at the same time. Then one must realize that one can just forget about trying to anticipate whether things will actually happen at the same time. There are so many things that impact that, from low level stuff like CPU scheduling and reordering to things you don't think about of changed use of the code in the future. So, my principle is, don't even think about if it will happen. Simply make sure it can't happen.

When it comes to implementing thread safety, it isn't necessarily so easy. I still find Java frustrating in this regard because of the tools it gives us to control this. They have so many quirks and often behave different than what I find intuitive, and this is where the difficulty lies. But, in most cases, it can be handled with one of three strategies:

• Use immutable objects where possible. Since immutable objects are "write protected" by design, that is, they never change after construction, they can be shared among threads without thinking of thread safety. The reason is that it doesn't matter if multiple threads reads the same memory at the same time as long as none of them writes to the memory. I just love immutable classes, but I hate some of the limitation Java puts on them like stupid constructor limitations that means you often have to use factory methods which fucks up inheritance, and the fact that you can't manually deserialize them (because you can't set final fields from the deserializer code - and you're not allowed to return a new instance from the deserializer either).
• Use some form of synchronization/serialized access to the shared objects. This can make the code complicated or lead to loss of efficiency because of waiting, so this often requires some planning when it comes to the design of the lock scheme. I often end up rewriting them 1-3 times :(
• Use memory barriers (volatile) for protection of single variables/fields. While it's very easy to do, it often turns out to be insufficient though, because you can't do a if (foo == 0) {foo = 1} with a volatile because it can be changed by another thread in the time window between where you read it and where you write it. Where this isn't the case though, they are great.

[SubJunk]

Yes there have been frustrations by all of us for various reasons that we have all talked about for a long time. I wish you luck with DMS, I hope it is a less frustrating experience for you. I have been reading about thread safety lately and I intend to keep it in mind when I am developing for UMS. Thanks for the notes.

[SubJunk]

I recently did a lot of work on ORG_PN values and verified it with a user who was having problems, so I believe this to be fixed. Feel free to reopen if it's not.