A better Video Coding Level implementation #1242

Sami32 · 2017-04-01T20:48:11Z

Implement the H.265 level renderer limitation
Correct a bug in the AVC level parsing
Log the HEVC level

The only purpose of that PR is to log this information, as some renderer, like Samsung 9 series, have a H.265 limitation as well, and that the video could be correctly triggered to transcoding if they need to be.

I leave a potential implementation in the video engines to some more skilled ones.

Sami32 · 2017-04-01T21:52:41Z

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

+		// // Value can look like "Stereo High@L4.1 / High@L4.1".
+		if (isNotBlank(substringBetween(lowerCase(value), "@l" , " /"))) {
+			final String avcLevel = substringBetween(lowerCase(value), "@l" , " /");
+		} else {


It's seem that it's not the correct way of doing that ?
final String seem not work in a conditional loop.

@Sami32 final means it can only be set once. However, the bigger problem here is scope. avcLevel will only exist in the scope of that if-block, so it won't exist as soon as the code leaves that.

Sami32 · 2017-04-01T22:18:05Z

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


 	public void setH264Profile(String s) {
 		synchronized (h264ProfileLock) {
 			h264Profile = s;
 		}
 	}
+
+	public void setH265Profile(String s) {
+		synchronized (h265ProfileLock) {


Is the reuse of String s correct in that case ?
If i understood correctly, it should be fine but i'm not sure at all.

@Sami32 You're not reusing it as it's separate methods. s only exists in that method's scope, so there's no conflict.

Using synchronization can however be a bit tricky, as you have to look at the context from which the method is called to make sure you don't create deadlocks.

Sami32 · 2017-04-02T20:26:13Z

I think that it ready to be review now.

Nadahar · 2017-04-02T21:12:44Z

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

@@ -156,6 +156,9 @@ public synchronized static void parse(DLNAMediaInfo media, InputFile inputFile,
 						if (!value.isEmpty() && media.getCodecV() != null && media.getCodecV().equals(FormatConfiguration.H264)) {
 							media.setAvcLevel(getAvcLevel(value));
 						}
+						if (isNotBlank(value) && media.getCodecV() != null && media.getCodecV().equals(FormatConfiguration.H265)) {


If this is read from the same MediaInfo value, is there any reason to store twice and have twice the locks etc?

Oh, you mean all the lock part that i copy past ?
I don't really know, as i just copy past and adapt, but you should be right.
But i'm afraid to make things worst if i try to delete some part.

@Sami32 That's ok, the important thing is that media.getCodecV() != null comes before media.getCodecV().equals....

What I was trying to say is that it seemed strange to take the same information from MediaInfo, stored in value, and store in two different places. But I see now that getAvcLevel() and getHvecLevel() produces different outcome, so I guess it's ok. Still, using one field to store this would mean you didn't have to expand the database etc to store this information twice.

I'd say that storing the "raw" value from Format_Profile in DLNAMediaInfo and the database would be better. The parsing of the value should be done in DLNAMediaInfo.getAvcLevel() and DLNAMediaInfo.getHevcLevel() instead. Parsing this will be so quick that doing it each time won't be noticable, and the benefit is that every DLNAMediaInfo instance kept in memory (which can be very many) and each database record will be smaller. It will also be enough with one set of locks.

This is where I feel like showing is easier than explaining, but I won't do that this time...

Sami32 · 2017-04-02T23:54:39Z

Ok, now i'm frustrated :-( as i cannot do as you explained, so feel free to commit and merge, as i really don't bother as long as it's done; but if you can make it optimal that'll be better.
Just delete my added attempt to do it if you need.

Sami32 · 2017-04-03T17:09:13Z

I cannot do better...

Nadahar · 2017-05-20T19:54:19Z

@Sami32 Are you talking to yourself? 😉

In any case, what happened to this? Did the H265 stuff we made ever get merged? I'm sorry, there are so many different "half done" projects now that I don't remember.

Sami32 · 2017-05-20T20:21:06Z

Yes, because i'm afraid to forget it.
And if i don't set my name, someone could think that i'm addressing to him ;)

No, it's in standby. I plan to work again on it, as the video issues already reported are probably linked to the incorrect/incomplet video level parsing ("unknow" level). 3D videos are also partially affected.

Since you've made a refactoring branch i didn't worked on that, but i'll need to check the point that you leave "suspension" and continue from your branch.
In fact it was when i was searching at your branch, that i'm still searching ;), that i worked on the subject that make me link here, and work on an other user issue met on the way.

SubJunk · 2017-07-30T02:27:32Z

Can this be closed or will there be more work on it?

Nadahar · 2017-07-30T03:16:47Z

@SubJunk There are some good improvements in here so I wouldn't delete it.

SubJunk · 2017-07-30T03:29:41Z

Cool, is there anything I can do to help it progress?

Nadahar · 2017-07-30T03:34:46Z

@SubJunk I don't remember exactly why or where it stranded, but you could study the approach and give a feedback on that. I think somebody will have to rebase it on latest master and resolve the conflicts if @Sami32 is going to be able to continue.

SubJunk · 2017-07-30T03:46:35Z

src/main/external-resources/renderers/DefaultRenderer.conf

 # Default: true
-H264Level41Limited = 
+H265Level41Limited =


It seems the old approach got mixed in here for H.265; this third option should be removed in favor of the one above it

SubJunk · 2017-07-30T03:47:44Z

src/main/external-resources/renderers/Sony-PlayStationVita.conf

@@ -21,7 +21,7 @@ SeekByTime = exclusive
 DLNALocalizationRequired = true
 TranscodeVideo = MPEGTS-H264-AAC
 TranscodeAudio = WAV
-H264Level41Limited = true
+H264LevelLimit = 4.1


This change will need to be made to all renderers that didn't have this line, too, unless the default value is set to 4.1 in which case this line can be removed

@SubJunk Since this was introduced in 6.5.x (?) and the default set to 4.1, I assume that was without actually checking the capabilities of the renderers. If my assumption is correct, I see no reason to "preserve" this arbitrary value. As you can see from #1418 this has already created problems for users, and for every report I assume there are hundreds that doesn't report it. In my opinion, it would be better to define those limitations that has actually been verified and leave the rest undefined.

SubJunk · 2017-07-30T03:49:13Z

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


 	// Database column sizes
 	private final int SIZE_CODECV = 32;
 	private final int SIZE_FRAMERATE = 32;
 	private final int SIZE_ASPECTRATIO_DVDISO = 32;
 	private final int SIZE_ASPECTRATIO_CONTAINER = 5;
 	private final int SIZE_ASPECTRATIO_VIDEOTRACK = 5;
-	private final int SIZE_AVC_LEVEL = 3;
+	private final int SIZE_VIDEO_FORMAT_PROFILE = 40;


Does it need 40 characters?

@SubJunk There seems to be a general misunderstanding of VARCHAR in our implementation. The old CHAR implementation allocates and use the space assigned to it. The is not the case for VARCHAR. These are allocated outside the main database table and only a pointer placed in the table. As such, they only use the space they need. The only point of restricting their size is to make sure they never get longer than that number.

Thus, I prefer to define them too large and hopefully never have to deal with trucated strings.

Oh cool, thanks for letting me know, I didn't know that

SubJunk · 2017-07-30T04:04:33Z

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

+		String profile = substringBefore(lowerCase(getVideoFormatProfile(), Locale.ROOT), "@l");
+		if (isNotBlank(profile)) {
+			return profile;
+		} else {


Just a formatting note, there's no need for the else here, or on line 2511, since it will return beforehand

SubJunk · 2017-07-30T04:07:38Z

src/main/java/net/pms/configuration/RendererConfiguration.java

-	public boolean isH264Level41Limited() {
-		return getBoolean(H264_L41_LIMITED, true);
+	public H264Level getH264LevelLimit() {
+		return H264Level.typeOf(getString(H264_LEVEL_LIMIT, null));


Maybe it should default to level 4.1?

@SubJunk agree that will be better to set the default value to level 4.1 and in this case the implemented logic in the DLNAResource line 976 must be changed to
} else if (media.isH264() && renderer.getH264LevelLimit() != H264Level.L4_1) {
and for all renderers where previously was set H264Level41Limited = false and this setting was deleted in this PR should be added H264LevelLimit = 5.2.

I'm not sure about this. Even though the old renderers were often limited to 4.1, is that really the case these days? I don't like to define defaults that isn't "logical", if no limit is defined for a given renderer it's most logical to me that there actually isn't a limit for that renderer.

@Nadahar I am not sure either but for me this is the safest way how to implement this PR. Users are using the UMS with the restriction to level 4.1 for the long time without complaining except users with renderers where was set H264Level41Limited = false. What we offer in this PR is only a different way how to decide which video will be streamed or transcoded.

Maybe this isn't the case anymore but AFAIK most renderers support 4.1 nicely. As @ExSport said in #1417 (comment) it can cause real problems the other way too (and it did) so either way we will inconvenience some users.
I think 4.1 is a safer default than blank though because it's better to have transcoded video than no video.

Edit: And if we ever feel the motivation to make these more accurate, usually the device manuals have that information in them. Though I should note that information can be inaccurate, like sometimes they say they support 5.1 but then they didn't take into account subtitles too and the device CPU can't handle both things at once so it stutters... Manual testing is the best way to know

@Nadahar I didn't look at it from this angle and I must say that you are right.

I don't agree with that, in fact our top competitor Plex aggressively transcodes on many renderers - they don't even have many specific renderer profiles. I think if a user has broken playback they will usually just uninstall us and go find something that works. Sometimes we get users reporting these things but that's not something to rely on.

Anyway I'm outvoted so go ahead :)

I essentially only logged on to reduce my mail spam by unwatching, but couldn'thelp myself. This must be one of the stupidest things I've ever heard when working with software design. As @SubJunk says normally if playback fails the user will just say "oh this doen't work". He won't write a bug report, since he will consider the program broken (failing to perform it's main task). He is more likely to write a report if seeking fails since that is a feature.

@SubJunk It's nothing that I feel that strongly about, I was just explaining my stance and why I did like I did. I've seen many examples on people being annoyed by needless transcoding. Some will probably turn away in either case, which is why this should be set correctly - not rely on the default.

One thing that also weighs in for me and that probably only is a "me thing" is that I feel it is "logically wrong" to set a default that is just a "random guess". I value "quality of information", so I like to make sure that whatever information I have is reasonably accurate, and prefer knowing that I don't know over using a guess that seems like real information to me. I realize that this is very likely to be something that is just important to me, and that the users won't care about this at all.

When it comes to Plex I don't really look at what they are doing. I don't consider Plex a "competitor" since it's payware. I tried the free version for a little bit a long time ago, and it annoyed the hell out of me with all it's lack of configuration/decisions being made for you. I know that for all the Apple users out there, this is appealing - but I think it's important to note what most companies seems to miss today as they rush to copy Apple - that this isn't the whole market. It's the most ignorant part of the market that appreciates having no choices, as they don't know how to make the decisions and rely 100% on the default behaviour. However, given how "difficult" UMS/PMS is and has always been, I doubt that's our current user base. If you just cater to the ignorant you will alienate the informed. That said, this is a general view of mine, that isn't particularly relevant to this decision, but it explains why I never look to what Plex does. For all I care, they cater to a different user base.

@SharkHunter Can I do anything to help you unwatch? I honestly can't wait to get rid of your mostly stupid and useless comments. I have a very hard time believing that this is one of the stupidest things you've ever heard, as you most often say more stupid things yourself.

Your argument doesn't really hold water - the default will "alienate" some users whatever choice is made. You're right that most users won't report, but the question is what will affect most users and how bad. If most renderers can play a higher level so that a lot of users will be impacted by needless transcoding and all the negative aspects with that, I'd say the average user experience is worse limiting to 4.1. If most renderers can't play a higher level, the situation is the opposite. In addition, my argument was that for those few that do report issues, it's much harder to understand what's wrong if the default is too low - so the chance that we will ever know about it and be able to correct it is much smaller.

My assumption is that most users don't even know what transcoding is. They are just installing a program that serves video to their device. If they don't see video on the device then our program is broken. If they see it, it works.
Our advanced users are capable of seeing that there is transcoding happening and maybe they don't like hearing the fan in the computer make more noise than usual or slower seeking, but as advanced users they can be the ones who submit the better renderer configs to us to reduce the transcoding.

So from my perspective we will make the most people happy by succeeding at playing video more often.

About the comment "given how difficult UMS/PMS is", you're right but it has always been my goal to make it easier for new users and we have made good progress on that. I hope we make more so that we can still offer the advanced features to opt-in to while "just working" for our Plex-lovers :)

SubJunk · 2017-07-30T04:08:40Z

Ok, I have left some notes. Overall it looks like a good change

http://www.samsung.com/us/support/answer/ANS00049952/ In the purpose to log for debugging and transcoding trigger, as some renderers have H.265 limitations as well. I leave a potential implementation in the video engines to more skilled ones. Conflicts: src/main/java/net/pms/dlna/LibMediaInfoParser.java

valib · 2017-11-23T21:40:05Z

@Sami32 why did you closed this PR? Will you continue on it? I rebased it to the latest master to be ready to merge after some requested changes asked by @SubJunk.

Sami32 · 2017-11-24T02:07:23Z

@valib I don't remember, perhaps a bad mood...
Not right now, as i'm working on other stuff.

I think remember that something needed to be verified and i saw one part of the code that could be done more effectively.
This idea come from users having reported video level bugs: #1170

SubJunk · 2018-03-27T04:41:28Z

This was a good change

Nadahar · 2018-03-27T14:10:31Z

@SubJunk Many good things have been made that has simply stranded because you don't respond/ignore them.

This was actually made 1 year ago now, and I sure don't remember anything of this now. I doubt @Sami32 does either. It shouldn't be hard to fix and merge this, but I suspect you'll have to do that yourself now.

SubJunk · 2018-03-30T03:09:32Z

If you scroll up a bit you'll see I did respond to this and even did a code review last year...

Nadahar · 2018-03-30T17:05:22Z

@SubJunk We made this between April 1 and April 8 2017. Your first comment here was 30 July 2017. I know that you've reviewed it since then, and that's all fine and well. I can't speak for @Sami32 and don't know exactly why he chose not to do anything further with that.

My point is that when there's no reaction or input on whether this is even wanted functionality for 4 months, people move on. I don't know about @Sami32, but the way I work I have everything related to a certain piece of code/PR in my head when I work with it, and can easily make changes or answer questions about it. When some time has passed, this has been replaced by something else I've done and I no longer know why decisions were made or if there is something that still needs to be done or handled in some particular way.

Refreshing my memory so that I'm up-to-date on a months old PR means that I basically have to study my own code and "relearn" information which I already knew. I've had to do that over and over again here because interest/feedback is lacking. I've come to a point where I'm not willing to do that anymore, when the reason simply is laziness from another person.

If this had been handled in a timely manner, that is within a couple of weeks after completion, I'm sure this would have been merged a long time ago. Now we're in the situation where somebody has to do the job of regaining overview over the changes, and I'm simply saying that I won't be me. Considering how silent @Sami32 has been on this, I assume that he feels the same way.

SubJunk · 2018-04-02T02:09:42Z

I appreciate your frustration but I'm not here to, nor am I interested in, micromanaging the code. You guys have the ability to review and merge your own branches and I think you did back then too, and I encourage it for things that don't change huge parts of behavior of the program.

I'm not always around here but I have at least done my best to give you guys the ability to have autonomy, which is a hard thing to do for me because I love this project!

I'm not the perfect project leader but I think we have a good program, there have been some nice reviews coming in for the v7 release too. I think we should be proud of what we have accomplished since you guys started contributing code to UMS.

SubJunk

It would be good to resurrect this and I think I will so this is really a note for myself - make sure to replace the tests that were removed

…ersalMediaServer into h265 # Conflicts: # src/main/external-resources/renderers/Samsung-9series.conf # src/main/java/net/pms/dlna/DLNAMediaDatabase.java # src/main/java/net/pms/dlna/LibMediaInfoParser.java # src/main/java/net/pms/remote/RemoteUtil.java

Sami32 added awaiting review bug:confirmed This bug has been confirmed to exist by a developer not ready labels Apr 1, 2017

Sami32 force-pushed the h265 branch 2 times, most recently from 315e94a to eaa0f97 Compare April 1, 2017 21:42

Sami32 commented Apr 1, 2017

View reviewed changes

Sami32 force-pushed the h265 branch 5 times, most recently from a99568a to 43e6a18 Compare April 2, 2017 19:07

Sami32 removed the not ready label Apr 2, 2017

Nadahar reviewed Apr 2, 2017

View reviewed changes

Sami32 added the not ready label Apr 2, 2017

Sami32 mentioned this pull request Apr 2, 2017

will not be streamed because the H.264 level is unknown #1170

Open

Sami32 force-pushed the h265 branch 2 times, most recently from 41f4de2 to b475eaa Compare April 2, 2017 23:41

Sami32 force-pushed the h265 branch 5 times, most recently from f38cf4d to 65cb9d8 Compare April 3, 2017 15:24

Sami32 removed awaiting review not ready labels Apr 3, 2017

Sami32 force-pushed the h265 branch from 67cadfa to 00002d0 Compare April 3, 2017 15:40

Sami32 closed this Apr 3, 2017

Nadahar force-pushed the h265 branch from 4f6d34b to 2b72a6e Compare May 20, 2017 19:52

SubJunk requested changes Jul 30, 2017

View reviewed changes

Sami32 added the not ready label Jul 30, 2017

Sami32 closed this Jul 30, 2017

Nadahar mentioned this pull request Nov 23, 2017

Update the H.264 level checking #1418

Closed

valib reopened this Nov 23, 2017

Sami32 added 3 commits November 23, 2017 18:52

Implemented H.265 validation and generalized H.264 validation

3492ca8

Update Samsung-9series.conf

d15df67

valib force-pushed the h265 branch from 2b72a6e to d15df67 Compare November 23, 2017 18:28

SubJunk requested changes Sep 12, 2018

View reviewed changes

SubJunk added 2 commits August 11, 2019 16:28

Fix

421b5ce

valib added this to To do 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

A better Video Coding Level implementation #1242

Are you sure you want to change the base?

A better Video Coding Level implementation #1242

Conversation

Sami32 commented Apr 1, 2017 • edited

Sami32 Apr 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sami32 Apr 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sami32 commented Apr 2, 2017

Nadahar Apr 2, 2017 • edited

Choose a reason for hiding this comment

Sami32 Apr 2, 2017 • edited

Choose a reason for hiding this comment

Nadahar Apr 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sami32 commented Apr 2, 2017

Sami32 commented Apr 3, 2017

Nadahar commented May 20, 2017

Sami32 commented May 20, 2017

SubJunk commented Jul 30, 2017

Nadahar commented Jul 30, 2017

SubJunk commented Jul 30, 2017

Nadahar commented Jul 30, 2017

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

SubJunk Nov 27, 2017 • edited

Choose a reason for hiding this comment

valib Nov 27, 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

SubJunk commented Jul 30, 2017

valib commented Nov 23, 2017

Sami32 commented Nov 24, 2017 • edited

SubJunk commented Mar 27, 2018

Nadahar commented Mar 27, 2018

SubJunk commented Mar 30, 2018

Nadahar commented Mar 30, 2018 • edited

SubJunk commented Apr 2, 2018

SubJunk left a comment

Choose a reason for hiding this comment

Sami32 commented Apr 1, 2017 •

edited

Sami32 Apr 1, 2017 •

edited

Sami32 Apr 1, 2017 •

edited

Nadahar Apr 2, 2017 •

edited

Sami32 Apr 2, 2017 •

edited

Nadahar Apr 2, 2017 •

edited

SubJunk Nov 27, 2017 •

edited

valib Nov 27, 2017 •

edited

Sami32 commented Nov 24, 2017 •

edited

Nadahar commented Mar 30, 2018 •

edited