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

Tune time bar update based on duration #5496

Merged
merged 4 commits into from
Mar 15, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ public class DefaultTimeBar extends View implements TimeBar {
private final CopyOnWriteArraySet<OnScrubListener> listeners;
private final int[] locationOnScreen;
private final Point touchPosition;
private final float density;

private int keyCountIncrement;
private long keyTimeIncrement;
Expand Down Expand Up @@ -242,6 +243,7 @@ public DefaultTimeBar(Context context, AttributeSet attrs) {
// Calculate the dimensions and paints for drawn elements.
Resources res = context.getResources();
DisplayMetrics displayMetrics = res.getDisplayMetrics();
density = displayMetrics.density;
fineScrubYThreshold = dpToPx(displayMetrics, FINE_SCRUB_Y_THRESHOLD_DP);
int defaultBarHeight = dpToPx(displayMetrics, DEFAULT_BAR_HEIGHT_DP);
int defaultTouchTargetHeight = dpToPx(displayMetrics, DEFAULT_TOUCH_TARGET_HEIGHT_DP);
Expand Down Expand Up @@ -447,6 +449,11 @@ public void setAdGroupTimesMs(@Nullable long[] adGroupTimesMs, @Nullable boolean
update();
}

@Override
public int getTimeBarWidthDp() {
return pxToDp(density, getWidth());
}

// View methods.

@Override
Expand Down Expand Up @@ -835,4 +842,8 @@ public static int getDefaultPlayedAdMarkerColor(int adMarkerColor) {
private static int dpToPx(DisplayMetrics displayMetrics, int dps) {
return (int) (dps * displayMetrics.density + 0.5f);
}

private static int pxToDp(float density, int px) {
return (int) (px / density);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import android.os.SystemClock;
import android.support.annotation.Nullable;
import android.util.AttributeSet;
import android.util.Log;
import android.view.KeyEvent;
import android.view.LayoutInflater;
import android.view.MotionEvent;
Expand Down Expand Up @@ -196,6 +197,9 @@ public interface VisibilityListener {
public static final int MAX_WINDOWS_FOR_MULTI_WINDOW_TIME_BAR = 100;

private static final long MAX_POSITION_FOR_SEEK_TO_PREVIOUS = 3000;
private static final int DEFAULT_UPDATE_DP = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: How did you choose this 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.

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.

private static final long MIN_UPDATE_FREQUENCY_MS = 50;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

private static final long MAX_UPDATE_FREQUENCY_MS = 1000;

private final ComponentListener componentListener;
private final View previousButton;
Expand Down Expand Up @@ -832,21 +836,39 @@ private void updateProgress() {
long delayMs;
if (player.getPlayWhenReady() && playbackState == Player.STATE_READY) {
float playbackSpeed = player.getPlaybackParameters().speed;
if (playbackSpeed <= 0.1f) {
delayMs = 1000;
} else if (playbackSpeed <= 5f) {
long mediaTimeUpdatePeriodMs = 1000 / Math.max(1, Math.round(1 / playbackSpeed));
int timeBarWidth = timeBar.getTimeBarWidthDp();

if (timeBarWidth == 0 || duration == 0) {
delayMs = MAX_UPDATE_FREQUENCY_MS;
} else {
// Calculate how many updates needs to be done with DEFAULT_UPDATE_DP
// to fill up the time bar
int numberOfUpdates = timeBarWidth / DEFAULT_UPDATE_DP;

// Calculate the designated update interval, taking duration into consideration as well
long mediaTimeUpdatePeriodMs = duration / numberOfUpdates;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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. ]


// Calculate the delay needed from the current position until the next update is due
long mediaTimeDelayMs = mediaTimeUpdatePeriodMs - (position % mediaTimeUpdatePeriodMs);

// If the delay would be too small, then skip the next update
if (mediaTimeDelayMs < (mediaTimeUpdatePeriodMs / 5)) {
mediaTimeDelayMs += mediaTimeUpdatePeriodMs;
}
delayMs =
playbackSpeed == 1 ? mediaTimeDelayMs : (long) (mediaTimeDelayMs / playbackSpeed);
} else {
delayMs = 200;

// Calculate the delay until the next update (in real time), taking
// playbackSpeed into consideration
delayMs = playbackSpeed == 1 ? mediaTimeDelayMs : (long) (mediaTimeDelayMs / playbackSpeed);

// Limit the delay if needed, to avoid too frequent / infrequent updates
if (delayMs < MIN_UPDATE_FREQUENCY_MS) {
delayMs = MIN_UPDATE_FREQUENCY_MS;
} else if (delayMs >= MAX_UPDATE_FREQUENCY_MS) {
delayMs = MAX_UPDATE_FREQUENCY_MS;
}
}
} else {
delayMs = 1000;
delayMs = MAX_UPDATE_FREQUENCY_MS;
}
postDelayed(updateProgressAction, delayMs);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,13 @@ public interface TimeBar {
void setAdGroupTimesMs(@Nullable long[] adGroupTimesMs, @Nullable boolean[] playedAdGroups,
int adGroupCount);

/**
* Get width of time bar in dps
*
* @return Width of time bar in dps
*/
int getTimeBarWidthDp();

/**
* Listener for scrubbing events.
*/
Expand Down