[mm_half3]

Hello,

Kind of new to xbmc, but learning every day. The ability to customize the software through skins and code is really nice. One of the first modifications I wanted to do was to get the Aspect Ratio of a movie to display when the movie info comes up. The skins already displayed this information, but it was limited to the set defined in StreamDetails.cpp. My setup is a constant height projector, so having correct Aspect Ratios in the movie info section is a function that helps out a bunch, since I don't have to go to another source (disk, internet, etc..) to see what preset to select on my projector to get it to display correctly.

I added AR definitions in StremDetails.cpp for 1.37, 2.40, and 2.76, then compiled binaries for macOsX and Ubuntu. After a mod to the skins Textures.xbt file to add the new png files, and adding the movie Aspect Ratios to the movie.nfo files, things were starting to look pretty good. However, there is still a problem cropping up. DVDPlayer is modifying the xbmc AspectRatio database field for a movie when a movie is played. Typically you would think this is not a problem, since DVDPlayer is more likely to have the true Aspect Ratio vs. what is scrapped to a nfo file...but for ripped blurays in 2.35, 2.40, or any others that encode the black bars into the movie file, the player can interpret the AR to be different than what the movie was shot in, and as such when it updates the xbmc AR field to reflect that, the AR in the movie info is now inaccurate. Well technically it is probably accurate since the black bars are in the movie file, but not for the purposes of users like me who have a constant height setup. The nfo AR can be reloaded into the xbmc AR field when the movie details are rescanned, but a preferred way would be to not have DVDPlayer change the AR field in the database.

I had been trying to trace through the xbmc code to see where updating the AR might be blocked, without much success. Not as easy as finding where to add new ARs to xbmc . One of the searches on the forum turned up this Post where the user explains where the database media flags gets updated, and I thought that might be exactly what I wanted to review to see how to prevent the AR from getting modified by DVDPlayer.

Line 4430 in Application.cpp has comments saying "update with stream details from player, if any", the users post says the player will check to see if there are any StreamDetails for the video, and if there aren't any, then the database will be updated with the current values from the player. My take on the post and the code after 4430 is that the GetStreamDetails call is not returning a value (or valid value) for the Aspect Ratio, and it in turn updates the xbmc database fields with what the player has.

Figured I would seek help on the forum to see if I am on the right track, or entirely off base on what can be done to get the player not to overwrite the database media flag values...specifically AR. Any help in accomplishing what I want to do, or comments on why stopping the player from updating the media flag database values might not be a good idea is greatly appreciated.

Thanks,

[mm_half3]

Good news....Think I found the code causing DVDPlayer to overwrite the media flags taken from .nfo files in the xbmc database. My previous post was done after reviewing Application.cpp from github repository source downloaded yesterday. I had a tarball of earlier xbmc-12.0 source and took a look at that. The code from the tarball source is different than the github repository, and after reviewing Application.cpp for it, I saw an if block at lines

Code:

//          if (m_pPlayer->GetStreamDetails(details->m_streamDetails) && details->HasStreamDetails())
//          {
//            CVideoDatabase dbs;
//            dbs.Open();
//            dbs.SetStreamDetailsForFileId(details->m_streamDetails, details->m_iFileId);
//            dbs.Close();
//            CUtil::DeleteVideoDatabaseDirectoryCache();
//          }

that makes writes to the VideoDatabase taking the data from StreamDetails. Was able to get a successful build using the modified Application.cpp, and after a few quick tests of viewing 5 minutes in two of the movies where the AR was being modified, it looks like commenting out those lines is going to do the trick. In the post I referred to earlier, CapnBry alluded that the current source might have changed since he coded things. After another review of the github version of Application.cpp I see a group of code where the VideoDatabase is modified, but there is a comment referring to a PVR in the code group, and not sure if this code block does the same thing since StreamDetails is not called in it. Will try commenting out those lines to see if the github source compiles with the mod, and has the desired effect of blocking the writes to the xbmc database later today.

Other than correcting incorrect or missing video flags from an nfo file in the xbmc database, wondering what other effects commenting out those lines of code might have. If anyone has insight into this, or where the github source mods the media flag entries of the xbmc db, let me know. Also, I am thinking it might be better to change the code so that it will only modify the database if there are not any values in there for a movie.

Thanks

[mm_half3]

Decided to accomplish this task by adding an if statement that would check whether a new flag is set or not, and if set, bypass the VideoDatabase writes. Was not sure what would be the best way to flag this, and went with checking to see if a file exists in special://home. Since most users might like the VideoPlayer over writing the VideoDatabase, the modded code defaults to this. A user must take an action to stop the writes. An easy (not sure if it is the best) way to do this is to create an add-on python script, and if the user installs/enables it, clicking on the script program in xbmc would create the flag file. I have already tested a very basic python script in the modded binary, and it seems to work on Osx...going to test it out in ubuntu soon. Also want the user to be able to revert back to allowing VideoDatabase writes when a video is played. Another basic python script could be created to delete the file....my hope is both options can be presented to the user in one add-on script, so the same script toggles back and forth between writing and not writing.....but will need to research in the python add-on script development forum some for that. For those interested the code segment modified in Application.cpp is here:

Code:

if (!item.IsDVDImage() && !item.IsDVDFile())
      {
        CVideoInfoTag *details = m_itemCurrentFile->GetVideoInfoTag();
        // Save information about the stream if we currently have no data
        if (!details->HasStreamDetails() ||
             details->m_streamDetails.GetVideoDuration() <= 0)
         {

          // define location of file used to flag nfo file details exist and don't overwrite them
          // then check to see if flag exists and if so write debug output to verify value of useNFOflags
          // and bypass writes to video database by player.
          //
          CStdString useNFOflags = "special://home/keep_media_flags";

          if (!CFile::Exists(useNFOflags))
                        if (m_pPlayer->GetStreamDetails(details->m_streamDetails) && details->HasStreamDetails())
                                {
                                        CVideoDatabase dbs;
                                        dbs.Open();
                                        dbs.SetStreamDetailsForFileId(details->m_streamDetails, details->m_iFileId);
                                        dbs.Close();
                                        CUtil::DeleteVideoDatabaseDirectoryCache();
                                }
          }
      }

Testing on a mac using transparency! skin has gone pretty well, in that I now get persistent AR icons in the areas of the skin that displays movie info. Playing a movie does not modify them....also don't see anything suddenly not working in xbmc, that's always a good sign . Inside Video player all of the movies display the png used for 1.78, regardless of what the actual AR is. That issue is not as significant for me, so have not traced why....maybe later.

Let me know if you think there are ways to improve the code, or implement this. But remember, I am new to xbmc, and my programming skill set is very basic, so advanced mods are out of my scope. Based on the comments (if any), in this thread, will look into getting things formally presented into the gitbub source. As noted earlier the Application.cpp file used for this mod, is not the current github version. The VERSION file has fb595f23fbf4f4a4bc9297373f5f0138a1e01a9f as the commit number, I think it may be from the end of January. It looks like Application.cpp was modded in the github source download from earlier this week. Is it even appropriate to request a branch on older source trees?

[kaffekask]

I would suggest a setting in advancedsettings.xml instead of the file, so take a look at xbmc/settings/AdvancedSettings.cpp.

This is one of the top 5 problems that make me use MPC-HC as an external player, so I hope it will make it to Gotham.

[leechguy]

Indeed, instead of using a flag file (special://home/keep_media_flags) why not use a setting in the advancedsettings.xml file for this? That is what this file is for.

Also, I don't understand why you're working with sourcecode tarballs when you could be using git instead. If you want to submit your patch, you will need to use git anyway since all code submissions must be in the form of a pull request (see HOW-TO:Submit a patch).
As you have already noticed, the code in the XBMC repository is continuesly changing and using sourcecode tarballs it is impossible to keep up. Using git it it as easy as doing: git pull --rebase (this will set your changes aside, get the latest source from git and then apply your changes on top of the latest sourcecode).

Do yourself a favor and create an account on github, next learn how to fork the xbmc repository. Read Git Usage on the xbmc wiki, starting at chapter 4 and don't be intimidated by chapter 8 as you probably won't need that.
Finally, before you start making any changes, create a branch. Don't apply the changes to your master branch, but use a separate branch for your feature instead (ie. git checkout -b <your branchname>).

I by no means am a git guru but there are so many people out there that are. Lots of info is available. And even though I still have a love/hate relationship with git, I would say that it is all worth it in the end

[da-anda]

please no advanced setting as long as it's really really needed. We always prefer a intelligent default than yet another advanced setting we have to maintain for ages (we actually try to get rid of as many as possible). So my idea would be to rather make XBMC detect the real movie aspect ratio then the streams ratio by using the "crop black bars" feature to detect real movie AR. Not sure if this is reliable though. This is also just my opinion, no guarantee that the devs will like it either way.

[mm_half3]

The reason I used a tar file is because it was the first source download I found, before actually knowing about the github site for xbmc. After looking in this subforum, I started downloading the source using github. The readme for macosx was not up to date to the current source, so while waiting for a new readme (only took a few days), I worked on the tarball source, which would compile fine using the included readme file. The revised steps in the new readme file allowed successful builds, but damn if I can find where the newer source is writing to the xbmc VideoDatabase, and changing the Aspect Ratio values. Application.ccp calls a function SaveCurrentFileSettings() that looks like it should be doing the writing, it has a write line, dbs.SetVideoSettings(m_itemCurrentFile->GetPath(), g_settings.m_currentVideoSettings);, but disabling this has no effect on the AR's, they all get changed to 16:9 when any widescreen AR video is played. I am starting to think the 16:9 AR is a default set by the player.. If the code where the writes to the xbmc database can be found, a workable bypass for the githb source probably won't be too difficult.

I had been only using github to download the latest source, and never took the time to learn its software management functions, but thanks to leechguy's friendly prodding, went through a couple of turtorials, and have started to test mods in branches. Wish I would have done it earlier, probably could have saved a big chunk of time.


Today, I cleaned up the debugging lines used in Applicationcpp, and got a basic python add-on script that presents the user with two buttons, one that will create the flag file when clicked, and the other removes it. I may look into adding a check to see if there is an nfo file as another condition of bypassing the VideoDatabase writes, since the movie path is in the field BaseMoviePath field of the itemlist. Will research advancedsettings.xml, but given a post already against doing that, will have to see how using it pans out.

[mm_half3]

Been working on source pulled from github earlier in the week, and have made some progress. The AR in the player now reports the correct AR when a bluray video is playing, however, it is still overwriting the videodatabase AR value, with what looks to be a default AR 16:9. Strangely, initial modding of the github source has provided pretty much a transpose of the results seen in the older source tree after initial mods. After modding, the older tree prevents overwriting the xbmc db when a bluray video is played, but always shows the 16:9 AR icon in the player when a bluray video is played.

The code I found affecting how the AR icon is displayed in the player when a bluray video is active is in GUIInfoManager.cpp Turns out both source trees use the same code segments for what is displayed in the player, and it is in the VIDEOPLAYER_VIDEO_ASPECT: case element from a large switch statement. Modding this area has the older source now displaying AR icons based on the information in the videos nfo file, in the player, when viewing a bluray video. The AR from the nfo is persistent after the player is stopped or paused.


It looks like I may have found the code segment that could be modded to keep nfo AR info persistent in the newer source tree. The icons are displayed from code in the LISTITEM_VIDEO_ASPECT: case element. Setting the return for this to an AR value (2.40, 2.35, etc...) will cause all bluray videos to display the png file for the AR value, changing things from the default 16:9 png. Not real helpful in practice since it just changes one wrong default for another. I am stuck trying to find how to get the video folder path from inside the LISTITEM case statement. Tried various ways, including variations on how it was obtained in the VIDEOPLAYER_VIDEO_ASPECT case element, but no luck....the path is always coming up blank or with an invalid value. Probably not going to make much progress, unless someone is able to chime in with a way to get the path to the folder of a selected video while in a LISTITEM case element of fGUIInfoManager.cpp.

[capfuturo]

Good evening mm_half3/da-anda,

Are you still involved in working out a solution for this issue? I am wondering if this could ever be fixed so we can enjoy the real and correct aspect ratio flags of our movies,without having XBMC overwriting the values with the metadata it reads from media file on every play-back. I have posted my issue here to give some support and momentum to the finding of a fix for this, if you could please have a look.

All best,

CF

[mm_half3]

Hello,

To those interested in this feature, finally was able to port it over to the gotham source pulled from the xbmc repository on github. Will be sending a pull request to the master xbmc branch soon. Since this is my first attempt at sending a git pull request, thought It might be useful to summarize what I have done to lead up to the pull request, and include the commit message put in the commit that included the changes to xbmc/GUIInfoManager.cpp to implement the feature, in this thread to get feedback on whether the process I am using is an acceptable way to do this. Here goes:

Forked the xbmc gotham master
cloned my fork locally
created a branch to develop the changes required for the feature
built source and tested binary on MacOsX mountain lion in the branch
committed the one source file modified to implement the feature to my development branch
merged the development branch with my forked master branch

As far as I can tell things should be ready to send a pull request from my forked master to the xbmc master. Tests with the macOSX binary have gone well, want to test things on linux (ubuntu 12.04), before sending the pull, will probably complete that tonight...but confident it will build and run on linux since similar mods in the xbmc-12.0 source compiled and ran on linux just fine. Let me know if I missed something in the process. Below is the commit message..the amount of detail in it is because the git reference doc said it was important to include a detailed commit message for when it is reviewed by others who might consider it in a pull request. Feel free to comment on the commit message, more..less...etc.

Thanks,

Commit will prevent invalid Aspect Ratio value writes to xbmc movie info database

The genesis for the problem addressed in this commit is the bluray spec. It
includes the black bars of a scope movie in the blu ray disc movie files.
When a user creates a 1:1 unencoded rip of a bluray disc to use with xbmc the video file
also has the black bars. The algorithm used to calculate the Aspect Ratio by the default video player in xbmc
uses the entire video file, black bars with the movie content, thus causing all scope movies (2.35, 2.40, etc..)
to have a 1.778 calculated Aspect Ratio value. This incorrect value is written to xbmc's
movie information database, overwriting any valid scope Aspect Ratio already there from a movie.nfo file.
To be clear this issue does not effect how a movie is displayed, only what is displayed in the
movie information areas of xbmc during and after playback. It goes without saying the
code used to fix this problem is not the optimum way to address the problem, it is a temporary
fix to be used until a developer familiar with the code used to calculate the Aspect Ratio
in xbmc player is able to modify it to become "black bar aware". One suggestion was to check if
the code used to crop black bars in xbmc can be re-used to make the Aspect Ratio algorithm
"black bar aware".

The feature request addressed in this patch to xbmc/GUIInfoManager.cpp is described in
these xbmc forum threads:

http://forum.xbmc.org/showthread.php?tid...pect+Ratio
http://forum.xbmc.org/showthread.php?tid...pect+ratio

I created the first thread to discuss possible solutions, track progress of mods done in
xbmc-12.0 source code, and highlight difficulties in porting the working solution in xbmc-12.0
to xbmc-13.0. Significant progress was recently made in porting the fix to xbmc-13.0, this commit
was created for the xbmc master branch developers to review for inclusion to the xbmc-13.0 source code.


Below is a cut from comment lines added to xbmc/GUIInfoManager.cpp describing the source code changes:

File has been modified to prevent overwrites of valid Aspect Ratios taken from nfo files
when available. To activate the new code it requires user intervention to create a flag file, typically done
from a simple xbmc python add-on, but can be done manually if the user creates the file.
When the flag/file is removed, either by the python add-on script, or manual deletion by a user
the original code is used.

The code changes consisted of adding a function, getAspect_Ratio_nfo, to parse the Aspect ratio
from the nfo file, adding lines to define the active movie path within the function CGUIInfoManager::GetItemLabel,
and lines added to two case statements VIDEOPLAYER_VIDEO_ASPECT, to write the Aspect Ratio of the nfo file
to the database so it is displayed correctly in the information menus within video player during playback
and LISTITEM_VIDEO_ASPECT to write the Aspect Ratio of the nfo file to the info database so it is persistent after stopping playback.

One additonal note. Skins for xmbc need to be modified to include png files that will display the correct icon for scope
movies (2.35, 2.40. 2.55, etc..). It looks like the user ronnie's pull request to include the icons in the default
confuence skin was merged into the branch in 08 2013.

[Memphiz]

don't merge your branch to your master branch - but instead do the pull request directly from your developer branch.

[mm_half3]

don't merge your branch to your master branch - but instead do the pull request directly from your developer branch.

will do. Thanks Memphiz.

---

don't merge your branch to your master branch - but instead do the pull request directly from your developer branch.

My development branch still has all the object files from the build and the binary dmg file, but the only commit performed was the changed source code file...should I do anything to "clean" the branch before making the pull request? If my understanding of commit and pull is correct, the pull is a request to review the source mods from commits, and would think not, but want to be sure.

[Memphiz]

You will see which Files changed and which commits are there when starting the pull request - so you can Double check before the request gets fired. The object Files won't be visible at all if you didn't add them to git (which you should not).

[mm_half3]

Pull request #3701 has been sent for this problem. Thanks for the clarification Memphiz.

[capfuturo]

Link to PR #3701