Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File__analyze_streams.cpp - Change for 2.39:1 DisplayAspectRatio/Stri… #829

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

File__analyze_streams.cpp - Change for 2.39:1 DisplayAspectRatio/Stri… #829

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Source/MediaInfo/File__Analyze_Streams.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2507,7 +2507,8 @@ void File__Analyze::DisplayAspectRatio_Fill(const Ztring &Value, stream_t Stream
else if (DAR>=(float)2.15 && DAR<(float)2.22) DARS=__T("2.2:1");
Copy link
Contributor

@Sami32 Sami32 Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, looking at your commit i just saw that 2.2:1 line above and get suspicious. i couldn't resist to post a remark here.
It look to me that it should be 2.21:1

+The following fixed values are supported:
+@table @option
+@item 4/3
+@item 16/9
+@item 221/100
+@EnD table
+Any other value will result in square pixels being signalled instead
+(see H.262 section 6.3.3 and table 6-3).

Taken from FFmpeg.

EDIT:
https://en.wikipedia.org/wiki/Aspect_ratio_(image)

2.20:1 (11:5, 22:10): 70 mm standard. Originally developed for Todd-AO in the 1950s. Specified in MPEG-2 as 2.21:1, but hardly used.

For the record, some popular movies like "2001", "Lawrence of Arabia" or "Tomorrowland" have been releasized in such format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this potentially outside of the scope of the pull request/issue which was dealing with the 2.39:1 issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe. As i said sorry, i couldn't resist when i discovered it ;)
You are right, i should have open an other issue. I'll try to remember to do that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

else if (DAR>=(float)2.23 && DAR<(float)2.30) DARS=__T("2.25:1");
else if (DAR>=(float)2.30 && DAR<(float)2.37) DARS=__T("2.35:1");
else if (DAR>=(float)2.37 && DAR<(float)2.45) DARS=__T("2.40:1");
else if (DAR>=(float)2.37 && DAR<(float)2.40) DARS=__T("2.39:1");
else if (DAR>=(float)2.40 && DAR<(float)2.45) DARS=__T("2.40:1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really keep that 2.40:1 value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno, it could break compatibility with some tools that are expecting that value? Though you could say the same for my commit, but I feel like this commit is closer to fixing a 'bug'. What is your reasoning for removing 2.40? That it is not a standardised ratio? I'd be in favour of keeping it ..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because i'm always hesitant to leave not standardized stuff. Also if you are right that could break some program parsing code, as when we correct codec name or change the codec informations displayed, i consider that the tools, programs using MediaInfo have to adapt.

That said, in that case that doesn't hurt AFAIK and it is only one line of code so...

else DARS.From_Number(DAR);
DARS.FindAndReplace(__T("."), MediaInfoLib::Config.Language_Get(__T(" Config_Text_FloatSeparator")));
if (MediaInfoLib::Config.Language_Get(__T(" Language_ISO639"))==__T("fr") && DARS.find(__T(":1"))==string::npos)
Expand Down