| Wednesday, April 10th, 2024, 06:18 UTC | ||
| [06:18:51] | Steve-Goodey (Steve-Goodey!~steve@2a00:23c5:7d81:ed01:7a84:3cff:fedf:a99) has joined #mythtv | |
| [08:39:35] | ooshlablu (ooshlablu!~ooshlablu@2601:19c:417e:3a10:c26e:5fcd:46c5:62ca) has quit (Ping timeout: 260 seconds) | |
| [09:07:04] | SteveGoodey (SteveGoodey!~steve@2a00:23c5:7d81:ed01:7a24:afff:fe9d:c233) has joined #mythtv | |
| [09:21:30] | SteveGoodey (SteveGoodey!~steve@2a00:23c5:7d81:ed01:7a24:afff:fe9d:c233) has quit (Quit: Konversation terminated!) | |
| [09:21:56] | Steve-Goodey (Steve-Goodey!~steve@2a00:23c5:7d81:ed01:7a84:3cff:fedf:a99) has quit (Quit: Konversation terminated!) | |
| [10:36:27] | ooshlablu (ooshlablu!~ooshlablu@2601:19c:417e:3a10:8492:5f2e:60f6:d5c8) has joined #mythtv | |
| [12:51:20] | mad_enz (mad_enz!~mad_enz@2607:f2c0:e241:fe84:b3f0:867c:73b2:cb5a) has quit (Ping timeout: 260 seconds) | |
| [13:02:41] | mad_enz (mad_enz!~mad_enz@2607:f2c0:e241:fe84:cbd7:1ac3:db09:33fb) has joined #mythtv | |
| [13:03:27] | Steve-Goodey (Steve-Goodey!~steve@2a00:23c5:7d81:ed01:7a84:3cff:fedf:a99) has joined #mythtv | |
| [13:32:04] | stuartm (stuartm!~gbee@mythtv/developer/stuartm) has joined #mythtv | |
| [13:32:05] | Mode for #mythtv by ChanServ!ChanServ@services.libera.chat : +v stuartm | |
| [13:35:14] | stuartm: | So, unless I've lost my mind (which can't be ruled out), there is something fundamentally wrong with these templates (lines 78–105) and yet, I can't really explain why I'm the first to notice the breakage in more than 3 years: |
| [13:35:17] | stuartm: | https://github.com/MythTV/mythtv/blob/master/ . . . chrono.h#L78 |
| [13:35:42] | stuartm: | Can anyone else see the problem here? Or am I mad? |
| [13:39:55] | stuartm: | you know, I've just noticed that this channel is practically dead – no-one uses IRC anymore? |
| [13:40:52] | stuartm: | Guess I will have to take this to the mailing list :/ |
| [13:57:22] | ooshlablu (ooshlablu!~ooshlablu@2601:19c:417e:3a10:8492:5f2e:60f6:d5c8) has quit (Remote host closed the connection) | |
| [13:59:26] | ooshlablu (ooshlablu!~ooshlablu@2601:19c:417e:3a10:70:a904:672b:4bd0) has joined #mythtv | |
| [14:48:08] | hampton: | What do you think is wrong with them? The std::enable_if_t clause? That means that they only exist when applied to floating point variables. |
| [14:52:21] | hampton: | The channel has been slow for a while. I'm usually logged in, but often afk. |
| [14:56:29] | hampton: | going afk again |
| [15:10:20] | stuartm: | the functions taken float as a input, inside they are casting to int_64t – we're losing precision |
| [15:12:03] | stuartm: | e.g. a frame duration at 0.033 seconds becomes 0 after passing through secondsFromFloat() |
| [15:13:37] | stuartm: | less of a concern perhaps for microsecondsFromFloat() or millisecondsFromFloat() since the errors will generally be lower, except when they are compounding e.g. when calculating total duration by combining the durations of each frame |
| [15:16:34] | stuartm: | I first noticed the issue because it completely breaks caption extraction – frame durations are using in the timing, at some point we convert to std::chrono (not sure why QT equivalents were used?) and in the process a number of things are broken – captions don't extract at all because the current time (based on running total of frame durations) becomes zero |
| [15:18:26] | stuartm: | however these same functions are used in a lot of other places so might be causing a lot of other more subtle issues, I've not really dug into that yet |
| [15:19:39] | stuartm: | seems like these should really be converting everything to microseconds first and then converting to the chrono type to avoid this |
| [15:23:20] | stuartm: | @hampton Sorry David, I hadn't noticed until just now that you were the author of this code. Please take nothing I'm saying as a criticism of your work. I'm glad there are still people taking an interest in working on MythTV and keeping it going |
| [15:47:38] | SteveGoodey (SteveGoodey!~steve@host86-133-222-21.range86-133.btcentralplus.com) has joined #mythtv | |
| [15:55:11] | ciphrCat (ciphrCat!~ciphrCat@user/ciphrcat) has quit (Quit: outta here) | |
| [16:04:38] | stuartm: | btw, regarding that comment about using QT, ignore that, forgetting that the best resolution there is only milliseconds – which technically would probably still be fine for everything video related but there's no equivalence to std::chrono |
| [16:17:38] | peterbennett (peterbennett!~peter@216.208.179.90) has joined #mythtv | |
| [16:30:35] | peterbennett (peterbennett!~peter@216.208.179.90) has quit (Ping timeout: 264 seconds) | |
| [16:44:42] | peterbennett (peterbennett!~peter@216.208.179.90) has joined #mythtv | |
| [16:55:33] | peterbennett (peterbennett!~peter@216.208.179.90) has quit (Ping timeout: 252 seconds) | |
| [17:14:56] | hampton: | I don't have a problem with secondsFromFloat(0.033) returning 0. Unless you'd rather it round up to 1? I would have a problem if millisecondsFromFloat(0.033) returned 0. Sounds like I might have made a mistake and the subtitle code should be calling millisecondsFromFloat? Although ISTR that all caption timestamps were in centiseconds? |
| [17:16:04] | hampton: | Is this internal subtitles or external file subtitles? |
| [17:21:03] | hampton: | Oh, I think I understand. |
| [17:22:04] | hampton: | You'd like secondsFromFloat(0.033) to return something of type std::chrono::milliseconds containing the value 33. |
| [17:30:36] | hampton: | When I wrote those macros I was thinking that the units designation in the name applied to both the units of the intput and the output. They were really just shorthand for having to write "std::chrono::blah(static_cast<int64_t>(value))" a whole bunch of places. I hadn't considered using them with different units for input and output, i.e. converting a float in seconds to a std::chrono::milliseconds, although that could also be a valid i |
| [17:30:36] | hampton: | nterpretation of the names. |
| [17:33:41] | hampton: | If we changed the definition to be that the input float argument was always in seconds, it would clean up a bunch of places that call "millisecondsFromFloat(1000 * somenumber)", although not all of them are written with a 1000 constant. |
| [20:15:52] | peterbennett (peterbennett!~peter@216.208.179.90) has joined #mythtv | |
| [20:16:08] | stuartm (stuartm!~gbee@mythtv/developer/stuartm) has quit (Remote host closed the connection) | |
| [20:16:33] | stuartm (stuartm!~gbee@mythtv/developer/stuartm) has joined #mythtv | |
| [20:16:33] | Mode for #mythtv by ChanServ!ChanServ@services.libera.chat : +v stuartm | |
| [20:17:11] | stuartm: | IMHO if you're using a float to represent a value in seconds, we shouldn't be discarding anything after the decimal point. That's not intuitive. If you don't care about fractional values for any of these then I'd suggest that it is less likely to cause confusion if the functions were *FromInt instead? |
| [20:20:29] | stuartm: | At least for the caption related code I was looking at, this was originally represented with double, i.e. whoever originally wrote the code felt that precision mattered enough to have much higher precision. That changed with these commits to the lower precision float, and then we're discarding that entirely when converting to std:chrono – which itself ironically is capable of even greater levels of precision |
| [20:23:31] | stuartm: | TLDR Just because I have a variable holding values in 'seconds' doesn't mean I only care about whole seconds, the choice of using a float (or double) type implicitly or even explicitly suggests otherwise |
| [20:24:21] | stuartm: | double duration = 1 / fps + static_cast<double>(frame->m_repeatPic) * 0.5 / fps; |
| [20:24:22] | stuartm: | m_curTime += secondsFromFloat(duration); |
| [20:24:30] | stuartm: | became this: |
| [20:25:20] | stuartm: | double duration = 1 / fps + frame->repeat_pict * 0.5 / fps; |
| [20:25:22] | stuartm: | m_curTime += duration * 1000; |
| [20:25:44] | stuartm: | sorry, I got those two backwards, second version is the original code |
| [20:25:52] | stuartm: | m_curTime is a double |
| [20:26:30] | stuartm: | storing millisconds not seconds |
| [20:27:03] | stuartm: | so arguably it should have been millisecondsFromFloat() anyway and this is just a typo? |
| [20:27:37] | stuartm: | However I'd still make the case that the use of a double for m_curTime was to allow for sub-millisecond levels of accuracy |
| [20:28:24] | peterbennett (peterbennett!~peter@216.208.179.90) has quit (Ping timeout: 252 seconds) | |
| [20:33:41] | stuartm: | as noted, over the duration of a recording m_curTime would drift noticably from the correct value if we did elect to drop fractional values even fractions of a millisecond. For most broadcast framerates, if they followed the standards, this wouldn't matter much in reality but in my experience reality can be very different. |
| [20:39:47] | stuartm: | I'm rambling a little here, it's been a long day, but I hope I'm making some amount of sense here? |
| [20:43:37] | stuartm: | I can see what you're saying about the names, I can see how 'secondsFromFloat' might be read to mean that we want to convert from a float value to whole seconds. I can accept that. Yet it appears in at least one case this wasn't the intent of the original code prior to it's conversion. Maybe that's a one-off incident but I have to wonder if there are other places where similar bugs were introduced because it would be an odd choice to use a float/ |
| [20:43:38] | stuartm: | double if you only want to store whole integers |
| [20:51:29] | peterbennett (peterbennett!~peter@216.208.179.90) has joined #mythtv | |
| [21:00:33] | Steve-Goodey (Steve-Goodey!~steve@2a00:23c5:7d81:ed01:7a84:3cff:fedf:a99) has quit (Quit: Konversation terminated!) | |
| [21:21:44] | peterbennett (peterbennett!~peter@216.208.179.90) has quit (Ping timeout: 252 seconds) | |
| [21:32:29] | ooshlablu (ooshlablu!~ooshlablu@2601:19c:417e:3a10:70:a904:672b:4bd0) has quit (Ping timeout: 240 seconds) | |
| [21:32:54] | ooshlablu (ooshlablu!~ooshlablu@2601:19c:417e:3a10:70:a904:672b:4bd0) has joined #mythtv | |
| [21:41:54] | SteveGoodey (SteveGoodey!~steve@host86-133-222-21.range86-133.btcentralplus.com) has quit (Quit: Konversation terminated!) | |
| [22:52:21] | mad_enz (mad_enz!~mad_enz@2607:f2c0:e241:fe84:cbd7:1ac3:db09:33fb) has quit (Ping timeout: 272 seconds) | |
| [23:04:42] | mad_enz (mad_enz!~mad_enz@2607:f2c0:e241:fe84:34ba:ad5c:18c3:c6a1) has joined #mythtv | |
IRC Logs collected by
BeirdoBot.
Please use the above link to report any bugs.