Tune time bar update based on duration

[szaboa]

Hi @tonihei , @ojw28
I've started to implement the thing what we've discussed on #5040.

Two things are unclear where I would need some guidance:

1. I didn't quite understand some calculations (line 841-845). Why exactly we need those? How can we include them into the current implementation if needed?
2. How this would work for live streams? How the duration works in that cases?

[tonihei]

mediaTimeUpdatePeriodMs is the number of media time seconds needed until one real time second passed (that's our designated update interval). For example for playback speed 0.5, this is 500ms.
mediaTimeDelayMs is the media time delay needed from the current position until the next update is due. Let's say our position is 102300ms, then we need to wait 500 - (102300 % 500) = 200ms in media time until it makes sense to update.
delayMs is then calculated as the real time corresponding to the media time we'd like to wait. So in the example, we would delay the update by 200/0.5 = 400ms.

I agree that is is very complicated and took me a while to figure out what's going on here. Feel free to simplify this with your change, but keep in mind different playback speeds when doing this.

The duration of live windows is either C.TIME_UNSET in which case we don't display a progress at all (because we don't anything about it), or it's the duration of the current live window and will change from time to time. For this purpose it's fine to assume that the live duration works the same as a VOD duration and you can also assume the duration itself won't change (even if the live window moves).

[szaboa]

@tonihei

1. I think I understand now, thank you.
   I've updated the calculations to follow this existing logic (and take the current position into consideration). I don't know how this could be simplified, however I've added some comments to be easier to understand. Please have a look on the changes and let me know if something needs to adjusted.

2. Got it!

[tonihei]

General approach looks good, thanks! I've added some suggestions to the code for further improvements. Please take a look.

[tonihei]

Can you make that final and move up to the other final variables?

[tonihei]

Can you add dp to the name, i.e. getTimeBarWidthDp?

[tonihei]

Isn't (float) densityDpi / DisplayMetrics.DENSITY_DEFAULT the same as displayMetrics.density? See method dpToPx avove. If so, it's probably better to use the same conversion such that both methods obviously do the same thing, just the other way round.

[szaboa]

Yes, it's the same, I can use that.

[szaboa]

But I can't pass the DisplayMetrics instead of density, because the width of the view can't be obtained yet in the ctr.

[tonihei]

It seems we already use a minimum delay of 200ms for the cases where the playback speed is > 5.0f. Can you change that accordingly, and also use the same constants in updateProgress()?

[szaboa]

Not sure if I understood correctly. Should I change MIN_UPDATE_FREQUENCY_MS to 200ms? The other half is done.

[tonihei]

Thinking about it again, 50ms might actually be a better minimum value. :)
After your change. we'll only use such low values for very short video durations and given that the human eye can easily see 20 updates per second, this seems like a good value to get smooth movements.

[tonihei]

Just curious: How did you choose this value?

[szaboa]

By checking a couple of values, how it looks visually. 20dp was too high and the progress jumped way too much. In my opinion of course.

[tonihei]

I think that should happen at the very end after taking playback speed into account. Otherwise the playback speed may still scale the delay to very large or small values.

[tonihei]

Be aware though, that the duration can be 0 (e.g. for live streams with unset duration).

[tonihei]

Same is probably true for the timeBarWidth? Could be 0 if the view is embedded in a strange way where the the timebar isn't really visible anymore.

[tonihei]

These test for speed <0.1 and speed > 5.0 where specifically chosen because all speed values between 0.1 and 5.0 would result in a delay between 200ms and 1000ms anyway.

As you now limit to the delay in another way and based on other criteria, I think you should be able to just remove these extra tests and let all cases go through the central code.

[szaboa]

Done, let me know if any further changes are needed :)

[tonihei]

Thinking about it again, 50ms might actually be a better minimum value. :)
After your change. we'll only use such low values for very short video durations and given that the human eye can easily see 20 updates per second, this seems like a good value to get smooth movements.

[tonihei]

Sorry for being nitty about this, but this can still result in divisions by zero. For example if timeBarWidth==3. The same is true for the other two divisions on line 852 and 861.

Maybe the easiest solution is to move this code to a separate private static method getTimeBarUpdateDelayMs. Then you do the steps and return early if an intermediate value is outside the valid range. This avoids a lot of nested if conditions. Something like:
int numberOfUpdate = ...
if (numberOfUpdates <= 0) {
return MAX_UPDATE_FREQUENCY_MS;
}
etc.

[ We usually put static methods below non-static ones, so please move that new method down to the other static methods. Thanks. ]

[szaboa]

Moved the calculations to a separate method, and added some verifications. Let me know if it's OK or something else needs to be adjusted.

[tonihei]

Thanks! Looks good now. I'll merge this change next week.

[tonihei]

Going to merge this PR soon. Note that we did some updates to your PR:

• We moved part of the logic into DefaultTimeBar so that the return value is independent of how the time bar actually looks like (e.g. doesn't rely on it being horizontal).
• Removed the DEFAULT_UPDATE_DP because the only reason for this is to limit the number of updates. The same can be achieved by setting the minimum update interval.
• Also made the minimum update interval configurable in the view and with a method. That is because we had some discussion around the best value. Using something like 33ms (=30fps) gives a nice smooth movement, but increases CPU usages quite a bit while the time bar is visible. So we went for a default of 200ms, which corresponds more to a step-wise update with no serious CPU increase. If you prefer the smoother version and you don't care about the CPU because the time bar is only visible for a short amount of time anyway, you can adjust this value to your preference.

[szaboa]

Alright, sounds good!