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.

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

[RFC] Get rid of frames-(m)sec conversion #13288

Merged
merged 6 commits into from Jan 10, 2018

Conversation

@Voyager1
Copy link
Member

commented Jan 2, 2018

Description

A relatively small change to isolate the repeated conversions between frames (aka 1/75th of a second) and seconds or milliseconds. CFileItem's startOffset and endOffset are expressed in "frames" so there's repeated conversion going on in several places. Plus having the conversion all over the place is error prone.
UPDATE
I've added code to remove the frames conversion and put all offsets in milliseconds. At first sight the code seems to work fine.

Note: the placement of the helper functions is currently CUtil but it's up for comments.

Motivation and Context

see above

How Has This Been Tested?

Code compiles. Some runtime testing done but needs more testing.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
xbmc/Util.h Outdated
@@ -204,6 +204,13 @@ class CUtil
*/
static int GetRandomNumber();

static int ConvertSecsToOffset(double secs) { return static_cast<int>(secs * 75); }
static double ConvertOffsetToSecs(int offset) { return offset / 75.0; }

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jan 2, 2018

Member

why do we use this somehow strage offset units?

This comment has been minimized.

Copy link
@Voyager1

Voyager1 Jan 2, 2018

Author Member

why do we use this somehow strage offset units?

this goes way back IMO. I think it's to have a precision of one frame, assuming 75fps. There are several places in the code that work with this. As I said, we can either clean this up a bit (like I'm proposing here, not too invasive) or go all the way and get rid of it completely (more risky).

This comment has been minimized.

Copy link
@notspiff

notspiff Jan 2, 2018

Contributor

specifically it was the resolution of some timer in use on og xbox. i don't recall all the dirty details but that's the source of the weird number.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jan 2, 2018

Member

I'd prefer the 2nd option. In current stage a little risk is acceptable. But it's your decision. Thanks very much for cleaning this up.

This comment has been minimized.

Copy link
@Voyager1

Voyager1 Jan 2, 2018

Author Member

don't know if I have the time for this - as I said this can go very deep and once beyond the simple conversion then the math with the frame counts kicks in and who knows how far that leads me. Probably not for inclusion in Leia v18. Edit: I'll take a look anyhow.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Let's try again now.
jenkins build this please

@Voyager1 Voyager1 changed the title Get rid of repeated constants in frames-(m)sec conversion Get rid of frames-(m)sec conversion Jan 3, 2018
@@ -456,7 +457,7 @@ int CCueDocument::ExtractTimeFromIndex(const std::string &index)
int secs = atoi(time[1].c_str());
int frames = atoi(time[2].c_str());

return (mins*60 + secs)*75 + frames;
return CUtil::ConvertSecsToOffset(mins*60 + secs) + frames;

This comment has been minimized.

Copy link
@Voyager1

Voyager1 Jan 3, 2018

Author Member

note to self - this needs correction

@Voyager1 Voyager1 changed the title Get rid of frames-(m)sec conversion [RFC] Get rid of frames-(m)sec conversion Jan 3, 2018
@Voyager1 Voyager1 force-pushed the Voyager1:offset-conversion branch from 17b77e6 to 3cf2e5f Jan 3, 2018
@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2018

@notspiff @FernetMenta the "weird" unit of 75 frames comes from the Cue-sheet standard: see https://en.wikipedia.org/wiki/Cue_sheet_(computing) .
I have added two commits to completely get rid of that unit (except when reading cue-sheets of course) so now start and end offsets are in milliseconds. That saves us a whole bunch of conversion. The only conversion still going on is between seconds (double) and milliseconds (int64_t).

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2018

jenkins build this please

1 similar comment
@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2018

jenkins build this please

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2018

once again, jenkins build this please

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2018

at last. Jenkins is good - just the usual OSX test failure...

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

@Voyager1 nice!

@ace20022

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Thanks, looks good at a glance.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

I've added code to remove the frames conversion and put all offsets in milliseconds.

Does that include the offset values held in the music DB? If not would it not be best to convert cuesheet values when originally read and saved msec values in the library too? But that would ential converting existing song data and a db bump. I will take that as a separate PR if liked.

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2018

Does that include the offset values held in the music DB

ha - I did not consider the existing ones. As of right now, they are in "frames" but new data will be stored in milliseconds. So existing data will be read as milliseconds thus will be wrong. Another option would be to store in db as "frames", ie. do a conversion upon read/write. But I like your suggestion (much cleaner IMO) and if you feel up to doing the db bump, that would be great!

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2018

@DaveTBlake I just want to clarify that three (not two) columns in the Song table will now be interpreted as milliseconds: iDuration, iStartOffset, iEndOffset.
UPDATE: this statement is incorrect. iDuration is actually in seconds.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

I get your thinking @Voyager1 but IMO Song table iDuration needs to remain as seconds. It is something all songs have, and for human use rather than player (which does not take any notice of what the library has anyway) - think track listings on the back of a CD case - and second accuracy is enough.

Music from cuesheets is not mainstream, and the only time that iStartOffset, iEndOffset is used. Despite the odd cuesheet standard using frames, I suspect that all offsets are to second accuracy - the example cuesheets I have seem have all been like that, and it seems a natural granularity for music. But in line with use elsewhere we can store as msec, after all they have never been the same unit as iDuration

Happy to do the db bump, I can roll it into some other small changes.

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2018

@DaveTBlake my bad... iDuration is (and has been) in seconds so indeed there's no change (I don't know what I was thinking!). Please go ahead with the bump. Next to the version increase, I think you'll just need an UPDATE SQL statement to multiply all iStartOffset and iEndOffset * 1000 / 75 (or * 40/3)

@Voyager1 Voyager1 force-pushed the Voyager1:offset-conversion branch from a556700 to d1d6fc5 Jan 9, 2018
@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

@DaveTBlake - since I didn't hear back I pushed an additional commit for the db bump. This way we have a complete PR that can be merged and tested.

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

jenkins build this please

@Voyager1 Voyager1 merged commit db046cf into xbmc:master Jan 10, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

Thanks for that @Voyager1, just not sure what I didn't reply to?

@DaveTBlake DaveTBlake added this to the L 18.0-alpha1 milestone Jan 10, 2018
@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

just not sure what I didn't reply to

you may have missed my message on slack. Never mind...

@popcornmix

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

Kodi no longer builds for me after this PR.
PRIi64 does not get defined by inttypes.h with gcc 4.9.3.
If I replace PRIi64 with PRId64 it builds again.

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

If I replace PRIi64 with PRId64 it builds again.

weird because jenkins had built it without issues. That said I think it would be ok to use PRId64 instead because it's the same for an output format specifier. Feel free to PR the fix. thanks!

@notspiff

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

it's likely due to interaction with #13336 (system.h removal).

@Rechi

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

jenkins told it works on top of #13336

@Voyager1 Voyager1 deleted the Voyager1:offset-conversion branch Jan 14, 2018
ksooo added a commit to ksooo/xbmc that referenced this pull request Jan 18, 2018
wiromare added a commit to wiromare/xbmc that referenced this pull request Jan 18, 2018
ksooo added a commit that referenced this pull request Jan 18, 2018
[video] Fix CGUIWindowVideoBase::GetResumeItemOffset after #13288 (on…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.