DTS-HD fix #1274

Sami32 · 2017-05-09T14:19:33Z

DTS-HD issues reported here:
#1258
#348
#1234 ?

Fixed the incorrect DTS-HD parsing
Fixed the broken audio delay implementation
Others related audio fixes / enhancements met on the way
Fix potential FFmpegVideo issues meet on the way

Nadahar · 2017-05-09T19:45:52Z

src/main/java/net/pms/dlna/LibMediaInfoParser.java

+						value = MI.Get(audio, i, "SamplingRate");
+						if (isNotBlank(value)) {
+							if (!value.contains("/")) {
+								currentAudioTrack.setSampleFrequency(getSampleFrequency(MI.Get(audio, i, "SamplingRate")));


@Sami32 Why do you get the value from MediaInfo again instead of just using value?

Because i've a much more limited global view/understanding of the code, not like you...

I can try to see what i can do.

@Sami32 This one is really simple, just replace the line above with

currentAudioTrack.setSampleFrequency(value);

Sorry i didn't understood what you meant. Now i do ;-) Yes, i didn't saw it, thanks.

Nadahar · 2017-05-09T19:48:47Z

src/main/java/net/pms/dlna/LibMediaInfoParser.java

 								currentAudioTrack.setBitsperSample(Integer.parseInt(value));
-							} catch (NumberFormatException nfe) {


@Sami32 It's bad to remove the catch here, Integer.parseInt() will throw if value is anything but a valid integer. That will terminate the thread and disconnect the renderer.

I agree, but i don't really know how try...catch work with if..else, so i limited my code to my actual skill. That's why.

@Sami32 I understand, but this now is a much bigger bug. You can read how it works here:
https://docs.oracle.com/javase/tutorial/essential/exceptions/handling.html

Sami32 · 2017-05-09T21:46:38Z

@Nadahar Thanks for the link, though i hardly understand all, most of the time i need examples from other tutorial or StackOverflow, to give me a better idea.

I still need to check a thing before make it ready, but feel free to review again ;-) Thanks.

Sami32 · 2017-06-24T13:05:49Z

@Nadahar Here the fresh informations concerning the bitrates meaning and reliability:

BitRate_Nominal: indicated (in the bitstream) average bitrate
BitRate_Maximum: maximal instantaneous bitrate; with MP4 based files, it is per frame and may be not good, should be per GOP, I need to change that in order to avoid high values as you see.
BitRate_Encoded: sometimes a stream is inside another one, e.g. audio and video are together in DV in MOV, and one can not be removed. BitRate is the bit rate of the audio in case it is demuxed from DV, BitRate_Encoded is 0 because it adds no bitrate to the file as it is already in the bitrate of the video part.
This is quite complicated and the output of MediaInfo is definitely not perfect for showing all differences.

Sami32

Review 2nd commit

Sami32 · 2017-06-24T14:43:45Z

src/main/java/net/pms/dlna/DLNAMediaAudio.java

+	public static final int NUMBEROFCHANNELS_DEFAULT = 2;
+	public static final int BITSPERSAMPLE_DEFAULT = 16;
+	public static final int AUDIODELAY_DEFAULT = 0;
+	public static final int SAMPLEFREQUENCY_DEFAULT = 48000;


I know that i'm the one that have choosen it, as it's well supported in 98%.
But now, i'm wondering if i shouldn't have choosen 44100 instead ?

@Sami32 I really don't have an opinion about what the default should be except that it should be what is most likely to be correct. My idea is always to try hard to do actual parsing and not having to rely on defaults. My "impression" is that 48000 is more common, but that's not based on any real facts but is just made up of which files I've happened to see most lately. If you'd asked me 10 years ago I would definitely have said 44100 though.

Sami32 · 2017-06-24T14:46:30Z

src/main/java/net/pms/dlna/DLNAMediaDatabase.java

@@ -258,19 +257,20 @@ public synchronized void init(boolean force) {
 				sb.append(", ID                INT              NOT NULL");
 				sb.append(", LANG              VARCHAR2(").append(SIZE_LANG).append(')');
 				sb.append(", TITLE             VARCHAR2(").append(SIZE_TITLE).append(')');
-				sb.append(", NRAUDIOCHANNELS   NUMERIC");
-				sb.append(", SAMPLEFREQ        VARCHAR2(").append(SIZE_SAMPLEFREQ).append(')');
+				sb.append(", NUMBEROFCHANNELS   OTHER");


small formatting (space)

Sami32 · 2017-06-24T17:43:40Z

src/main/java/net/pms/dlna/LibMediaInfoParser.java

+	public static int[] parseBitsPerSample(String value) {
+		if (isBlank(value)) {
+			LOGGER.debug("MediaInfo returned a blank bits per sample value, falling back to default");
+			return new int[0];


"returned a blank", i don't find this really correct or too much meaningful, as it can be empty or null.
"didn't returned a valid bits per sample value" ?

@Sami32 I interpret "not valid" or "invalid" as broader than blank. Blank to me means null or a string containing nothing or only whitespace. Empty means only null or an empty string. I guess I'm influenced by Apache Commons, as it's the definition they use. Invalid on the other hand is any value that's not "useful"/"meaningful" in the current content, and includes most non-empty string values as well.

Sami32 · 2017-06-24T17:55:57Z

src/main/java/net/pms/dlna/LibMediaInfoParser.java

+			return 0;
+		}
+
+		// examples of libmediainfo output (mediainfo --Full --Language=raw file):


Typo: The majuscule is missing.

@Sami32 Pardon my French, do you mean that the E in examples should be capitalized? I didn't write or change the text, I only moved it.

Yes.
Sorry, i thought that it was also the same english word. Now i remember that you said "upper case" 😳

Sami32 · 2017-06-24T17:56:50Z

src/main/java/net/pms/dlna/LibMediaInfoParser.java

+			return new BitRateMode[0];
+		}
+
+		// examples of libmediainfo output (mediainfo --Full --Language=raw file):


Same same for all this kind.

Sami32 · 2017-06-24T18:00:09Z

src/main/java/net/pms/encoders/MEncoderVideo.java

 					fakemedia.setWidth(1280);
 					fakemedia.setHeight(720);
-					audio.setSampleFrequency("48000");
+					audio.setSampleRate(48000);


Thanks, i did found the fact to have the two naming in the code was confusing, i needed trying to remember or search to check if it was the same value.

@Sami32 That was my idea as well, but I later discovered that while MediaInfo use "sample rate" DLNA use "sample frequency" so we won't get rid of the confusion whatever we do.

Sami32 · 2017-06-24T18:04:42Z

src/main/java/net/pms/formats/v2/AudioProperties.java

 	 *
-	 * @param mediaInfoValue libmediainfo "Video_Delay" value to parse.
+	 * @param returnDefault Whether to return the default value if not known.
+	 * @return The sample frequency in HZ.


Sami32 · 2017-06-24T18:23:21Z

src/main/java/net/pms/util/BitRateMode.java

+	public String toString() {
+		switch (this) {
+			case CBR:
+				return "Constant";


I know that you like verbose, and i could understand that it can be more meaningful.
But as you know, i like more CBR or VBR, it's more log readable to be. Too much text / verbose kill the informations; it was just a share of my point of view only, so i'll not bargain for a change.

@Sami32 You need a bigger computer screen. I have 3 screens, so I don't need everything to be in cryptic codes 😛

I see your point though and it's some truth to it, but I personally find CBR and VBR to be too similar so that I often misread them. IMO, C and V would be better. It might just be my eyes, but they do have some problems separating the two.

No need, i've big glasses and i only misread 1 and l 😛
I've an old 14" cathodic screen, but i have colors LOL

Ah, those were the days - when you could use the computer for hours and get a tan (or, atleast the sunburn). If I didn't know better, I wouldn't believe you. Nothing bad about CRTs though, I still miss them (except for the sunburn) because they could handle any resolution, had better refresh rates etc, But seriously, 14"? I do remember that those were standard back in the monochrome days. I'm sure you can go to the local trash dump and find a bigger screen. The problem isn't the thickness of your glasses, but the low resolution you're probably running. You get very little "screen area" in pixel dimensions, meaning that you can have very little information on the screen. I get claustrophobia just thinking about it.

Sami32 · 2017-06-24T18:47:40Z

src/main/java/net/pms/dlna/DLNAMediaAudio.java

-	public static final int SAMPLEFREQUENCY_DEFAULT = 48000;
+
+	/** The default sample rate used if the actual value is unknown */
+	public static final int SAMPLERATE_DEFAULT = 48000;


I cannot remember where i did a remark about this value, but i remember why i did chosen it, and still agree with it.
It was because MediaInfo have some limitations and when it doesn't give any values, most of the time it is 48000 Hz.

I keeped the 44100 for the no audio file commit that i've done, and become confused about that choice. red fish's memory symptome 😉

Sami32 · 2017-06-24T21:13:46Z

src/main/java/net/pms/dlna/DLNAMediaInfo.java

-		if (bitrate > 0) {
-			return (bitrate / 8);
+	// XXX This doesn't handle variable bitrates, nor does it adhere to DLNA which states that
+	// (for variable bitrates) the bitrate should be the maximum bitrate or if unknown, the maximum


As a side note, i'll said that AAC variant have the bitRate_Maximum, that will be reworked later in Mediainfo, and DTS VBR should be handled with the maximum value specified in the standard depending on its channels number. Most of the audio tracks have a CBR.
That was just my point of view.

Sami32 · 2017-06-26T23:07:44Z

@Nadahar It's amazing to see how too many media servers send the bitrate in bits/second, and it seem that the DLNA standard wasn't even clear for Cling developpers:
https://github.com/4thline/cling/blob/d9c3b7d188b744e7ae894fddb0d7251ca1bdd961/workbench/src/main/java/org/fourthline/cling/workbench/plugins/contentdirectory/ItemFormPanel.java#L138

(sample rate x bit depth x number of channels) / 8 + the average overhead bitrate
Bitrate or Bitrate_Nominal (the biggest of the two) from 1 video + 1 audio streams + 1 subtitle stream that will be used
Overall Bitrate as a fallback if available, as duration / size have serious limitations
DLNA formats maximum bitrate authorized per specifications
Or don't set it if too big incertitude is there, as it is optional and that the renderers seem to be too smart, for most of them ;-)

Here a link related to media server that you could find interesting.

As a general note, it seem that most of the media servers around implemented the UPnP flexible bitrate, at least partially:

The res@bitrate value should not be taken as sufficient from a QoS or other perspective to
prepare for the stream; The protocol used and the physical layer headers can increase the
actual bandwidth needed.

Concerning the average protocol / physical layer overhead bitrate, i understand why they give up:
http://web.cs.ucla.edu/classes/fall03/cs218/paper/Balk-ad_mpeg_w_band_est.pdf (page 7)

Nadahar · 2017-06-27T15:27:22Z

@Sami32 I'm not amazed that a lot of people get it wrong when they (UPnP) manage to name a property bitRate when it means byterate. Why would you look up "bitrate", it's pretty self-explanatory?

The rest of your post I don't understand very well. The points that you list, are they your points or a reference to some standard or project?

I really don't understand what there is to see in the linked "media server". I'm not going to read the whole document, so you need to give some indication of what to read. From what I can see it's about making some rules about UPnP AV format support, in many ways similar to DLNA.

The quote is directly from the UPnP standard and is quoted by me further up. Why do you repeat it, it doesn't say anything about the implementations, it's merely the standard itself which I have quoted and pointed to regarding QoS earlier.

When it comes to overhead, I'd say that there is no way a media server can know that. There are a lot of factors that come into play, some of which are specific to that local network and its configuration (like the MTU size that I mentioned earler). In Wifi networks there are a lot of corrupt packets that has to be retransmitted (and this depends on local factors, equipment quality, signal strength etc). One could argue that this retransmission is part of the "TCP overhead" as well, as it's handlet on the TCP level. It's simply impossible and unreasonable to find the real overhead. It looks like a very stupid move by DLNA to even mix overhead into the equation, but my guess is that they operate with some simple "standard values" for the different network/protocol/formats. Sadly we don't have these, so if we were to care about that part we'd need to make some pretty rough guesses.

When I look at the WireShark trace when using WMP server, I see that there's a small addition to the calculated bitrate (it seems like 30 bit/sec or something like that) for streamed content. However, for transcoded content the values are given without any such addition (the numbers are "round"). Mixing the overhead into the equation seems like a DLNA mess-up to me.

The thing to consider is probably: If you give a too low bitrate, some renderers will try to play the content and fail/lag/skip because they can't keep up. If you give a too high bitrate, some renderers probably won't try to play it because it's above some threshold. Say that you have a 320 bit/s ADTS file, and you announce it as 40004 (320030 bit/s). I'm pretty sure that some renderers will have hardcoded a limit of 40000 for that content, so they won't attempt to play the file. It's very unlikely that the small overhead is what makes the renderer unable to cope, so I think the best solution might be not to add the "estimated overhead" at all.

Sami32 · 2017-06-27T17:27:29Z

@Nadahar I agree, as i said that what already do UPnP without any issue, AFAIK.

But as i was thinking that you would absoletly make it perfect, so i just make this post 😉

As a side note, i don't have WMP but from what i saw in your Wireshark capture, many other things are "roundup" / "approximate", like durations or bitrates when they are not by a default fallback.

SubJunk · 2018-09-28T10:06:21Z

@Sami32 is this PR dead?

Sami32 · 2018-09-28T11:02:52Z

I'm afraid that it became overtimed for me.
That's said feel free to use it or close it.

SubJunk · 2018-09-28T21:58:16Z

Why delete Nadahar's comments?

I will keep this PR around since it seems like there are good things here

Sami32 · 2018-09-28T22:10:16Z

Because we did out of topic that are of no interest for anyone else.

SubJunk · 2018-09-29T02:32:56Z

Haha it was pretty funny to read though ;) I enjoy conversations like that too

SurfaceS · 2024-05-24T16:57:02Z

I don't see anything that is not implemented since at least V13 here.

Sami32 force-pushed the dtshd_parsing branch from 0beacf9 to 0f242fc Compare May 9, 2017 19:00

Nadahar reviewed May 9, 2017

View reviewed changes

Sami32 added the not ready label May 9, 2017

Sami32 force-pushed the dtshd_parsing branch from df8f0f8 to e504ea3 Compare May 9, 2017 21:37

Sami32 force-pushed the dtshd_parsing branch from fe35185 to d7b0d86 Compare May 9, 2017 23:45

Sami32 changed the title ~~DTS-HD fix part 1/3~~ DTS-HD fix attempt May 9, 2017

Sami32 changed the title ~~DTS-HD fix attempt~~ DTS fix attempt May 9, 2017

Sami32 added the bug:confirmed This bug has been confirmed to exist by a developer label May 10, 2017

Sami32 force-pushed the dtshd_parsing branch 6 times, most recently from a1d230a to c74aef9 Compare May 11, 2017 10:16

Sami32 added the enhancement label May 11, 2017

Sami32 force-pushed the dtshd_parsing branch 11 times, most recently from bfc071b to 3e96376 Compare May 11, 2017 19:51

Sami32 added awaiting review and removed not ready labels May 11, 2017

Sami32 commented Jun 24, 2017

View reviewed changes

Sami32 and others added 3 commits June 25, 2017 17:25

DTS-HD fix attempt

1d2d73f

work in progress

aa643de

work in progress

8286069

Nadahar force-pushed the dtshd_parsing branch from bbd743a to d1c9204 Compare June 25, 2017 15:26

work in progress

00fe263

Nadahar force-pushed the dtshd_parsing branch from d1c9204 to 00fe263 Compare June 25, 2017 15:48

Nadahar referenced this pull request Jul 13, 2017

Fixed splash screen icon

0b7bbe8

Nadahar mentioned this pull request May 10, 2018

DTS-HD parsing DigitalMediaServer/DigitalMediaServer#43

Merged

UniversalMediaServer deleted a comment from Nadahar Sep 28, 2018

valib added this to Review in progress in Universal Media Server May 28, 2020

github-actions bot added the conflicting label Jan 18, 2024

UniversalMediaServer deleted a comment from github-actions bot Jan 19, 2024

SurfaceS closed this May 24, 2024

Universal Media Server automation moved this from Review in progress to Done May 24, 2024

		currentAudioTrack.setBitsperSample(Integer.parseInt(value));
		} catch (NumberFormatException nfe) {

DTS-HD fix #1274

DTS-HD fix #1274

Conversation

Sami32 commented May 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sami32 May 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sami32 commented May 9, 2017 • edited

Sami32 commented Jun 24, 2017

Sami32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sami32 Jun 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sami32 Jun 24, 2017 • edited

Choose a reason for hiding this comment

Sami32 commented Jun 26, 2017 • edited

Nadahar commented Jun 27, 2017 • edited

Sami32 commented Jun 27, 2017 • edited

SubJunk commented Sep 28, 2018

Sami32 commented Sep 28, 2018

SubJunk commented Sep 28, 2018 • edited

Sami32 commented Sep 28, 2018

SubJunk commented Sep 29, 2018

SurfaceS commented May 24, 2024

Sami32 commented May 9, 2017 •

edited

Sami32 May 9, 2017 •

edited

Sami32 commented May 9, 2017 •

edited

Sami32 Jun 25, 2017 •

edited

Sami32 Jun 24, 2017 •

edited

Sami32 commented Jun 26, 2017 •

edited

Nadahar commented Jun 27, 2017 •

edited

Sami32 commented Jun 27, 2017 •

edited

SubJunk commented Sep 28, 2018 •

edited