Commit 855db95f by olly Committed by Ian Baker

Update surface frame-rate based on fixed frame-rate estimate

PiperOrigin-RevId: 350442843
parent 25ed6b12
...@@ -25,10 +25,8 @@ import java.util.Arrays; ...@@ -25,10 +25,8 @@ import java.util.Arrays;
*/ */
/* package */ final class FixedFrameRateEstimator { /* package */ final class FixedFrameRateEstimator {
/** /** The number of consecutive matching frame durations required to detect a fixed frame rate. */
* The number of consecutive matching frame durations for the tracker to be considered in sync. public static final int CONSECUTIVE_MATCHING_FRAME_DURATIONS_FOR_SYNC = 15;
*/
@VisibleForTesting static final int CONSECUTIVE_MATCHING_FRAME_DURATIONS_FOR_SYNC = 10;
/** /**
* The maximum amount frame durations can differ for them to be considered matching, in * The maximum amount frame durations can differ for them to be considered matching, in
* nanoseconds. * nanoseconds.
...@@ -44,13 +42,12 @@ import java.util.Arrays; ...@@ -44,13 +42,12 @@ import java.util.Arrays;
private Matcher candidateMatcher; private Matcher candidateMatcher;
private boolean candidateMatcherActive; private boolean candidateMatcherActive;
private boolean switchToCandidateMatcherWhenSynced; private boolean switchToCandidateMatcherWhenSynced;
private float formatFrameRate;
private long lastFramePresentationTimeNs; private long lastFramePresentationTimeNs;
private int framesWithoutSyncCount;
public FixedFrameRateEstimator() { public FixedFrameRateEstimator() {
currentMatcher = new Matcher(); currentMatcher = new Matcher();
candidateMatcher = new Matcher(); candidateMatcher = new Matcher();
formatFrameRate = Format.NO_VALUE;
lastFramePresentationTimeNs = C.TIME_UNSET; lastFramePresentationTimeNs = C.TIME_UNSET;
} }
...@@ -59,29 +56,8 @@ import java.util.Arrays; ...@@ -59,29 +56,8 @@ import java.util.Arrays;
currentMatcher.reset(); currentMatcher.reset();
candidateMatcher.reset(); candidateMatcher.reset();
candidateMatcherActive = false; candidateMatcherActive = false;
formatFrameRate = Format.NO_VALUE;
lastFramePresentationTimeNs = C.TIME_UNSET; lastFramePresentationTimeNs = C.TIME_UNSET;
} framesWithoutSyncCount = 0;
/**
* Called when the renderer's output format changes.
*
* @param formatFrameRate The format's frame rate, or {@link Format#NO_VALUE} if unknown.
*/
public void onFormatChanged(float formatFrameRate) {
// The format frame rate is only used to determine to what extent the estimator should be reset.
// Frame rate estimates are always calculated directly from frame presentation timestamps.
if (this.formatFrameRate != formatFrameRate) {
reset();
} else {
// Keep the current matcher, but prefer to switch to a new matcher once synced even if the
// current one does not lose sync. This avoids an issue where the current matcher would
// continue to be used if a frame rate change has occurred that's too small to trigger sync
// loss (e.g., a change from 30fps to 29.97fps) and which is not represented in the format
// frame rates (e.g., because they're unset or only have integer precision).
switchToCandidateMatcherWhenSynced = true;
}
this.formatFrameRate = formatFrameRate;
} }
/** /**
...@@ -113,6 +89,7 @@ import java.util.Arrays; ...@@ -113,6 +89,7 @@ import java.util.Arrays;
switchToCandidateMatcherWhenSynced = false; switchToCandidateMatcherWhenSynced = false;
} }
lastFramePresentationTimeNs = framePresentationTimeNs; lastFramePresentationTimeNs = framePresentationTimeNs;
framesWithoutSyncCount = currentMatcher.isSynced() ? 0 : framesWithoutSyncCount + 1;
} }
/** Returns whether the estimator has detected a fixed frame rate. */ /** Returns whether the estimator has detected a fixed frame rate. */
...@@ -120,6 +97,19 @@ import java.util.Arrays; ...@@ -120,6 +97,19 @@ import java.util.Arrays;
return currentMatcher.isSynced(); return currentMatcher.isSynced();
} }
/** Returns the number of frames since the estimator last detected a fixed frame rate. */
public int getFramesWithoutSyncCount() {
return framesWithoutSyncCount;
}
/**
* Returns the sum of all frame durations used to calculate the current fixed frame rate estimate,
* or {@link C#TIME_UNSET} if {@link #isSynced()} is {@code false}.
*/
public long getMatchingFrameDurationSumNs() {
return isSynced() ? currentMatcher.getMatchingFrameDurationSumNs() : C.TIME_UNSET;
}
/** /**
* The currently detected fixed frame duration estimate in nanoseconds, or {@link C#TIME_UNSET} if * The currently detected fixed frame duration estimate in nanoseconds, or {@link C#TIME_UNSET} if
* {@link #isSynced()} is {@code false}. Whilst synced, the estimate is refined each time {@link * {@link #isSynced()} is {@code false}. Whilst synced, the estimate is refined each time {@link
...@@ -134,9 +124,9 @@ import java.util.Arrays; ...@@ -134,9 +124,9 @@ import java.util.Arrays;
* #isSynced()} is {@code false}. Whilst synced, the estimate is refined each time {@link * #isSynced()} is {@code false}. Whilst synced, the estimate is refined each time {@link
* #onNextFrame} is called with a new frame presentation timestamp. * #onNextFrame} is called with a new frame presentation timestamp.
*/ */
public double getFrameRate() { public float getFrameRate() {
return isSynced() return isSynced()
? (double) C.NANOS_PER_SECOND / currentMatcher.getFrameDurationNs() ? (float) ((double) C.NANOS_PER_SECOND / currentMatcher.getFrameDurationNs())
: Format.NO_VALUE; : Format.NO_VALUE;
} }
...@@ -184,6 +174,10 @@ import java.util.Arrays; ...@@ -184,6 +174,10 @@ import java.util.Arrays;
return recentFrameOutlierFlags[getRecentFrameOutlierIndex(frameCount - 1)]; return recentFrameOutlierFlags[getRecentFrameOutlierIndex(frameCount - 1)];
} }
public long getMatchingFrameDurationSumNs() {
return matchingFrameDurationSumNs;
}
public long getFrameDurationNs() { public long getFrameDurationNs() {
return matchingFrameCount == 0 ? 0 : (matchingFrameDurationSumNs / matchingFrameCount); return matchingFrameCount == 0 ? 0 : (matchingFrameDurationSumNs / matchingFrameCount);
} }
......
...@@ -51,6 +51,31 @@ public final class VideoFrameReleaseHelper { ...@@ -51,6 +51,31 @@ public final class VideoFrameReleaseHelper {
private static final String TAG = "VideoFrameReleaseHelper"; private static final String TAG = "VideoFrameReleaseHelper";
/**
* The minimum sum of frame durations used to calculate the current fixed frame rate estimate, for
* the estimate to be treated as a high confidence estimate.
*/
private static final long MINIMUM_MATCHING_FRAME_DURATION_FOR_HIGH_CONFIDENCE_NS = 5_000_000_000L;
/**
* The minimum change in media frame rate that will trigger a change in surface frame rate, given
* a high confidence estimate.
*/
private static final float MINIMUM_MEDIA_FRAME_RATE_CHANGE_FOR_UPDATE_HIGH_CONFIDENCE = 0.02f;
/**
* The minimum change in media frame rate that will trigger a change in surface frame rate, given
* a low confidence estimate.
*/
private static final float MINIMUM_MEDIA_FRAME_RATE_CHANGE_FOR_UPDATE_LOW_CONFIDENCE = 1f;
/**
* The minimum number of frames without a frame rate estimate, for the surface frame rate to be
* cleared.
*/
private static final int MINIMUM_FRAMES_WITHOUT_SYNC_TO_CLEAR_SURFACE_FRAME_RATE =
2 * FixedFrameRateEstimator.CONSECUTIVE_MATCHING_FRAME_DURATIONS_FOR_SYNC;
/** The period between sampling display VSYNC timestamps, in milliseconds. */ /** The period between sampling display VSYNC timestamps, in milliseconds. */
private static final long VSYNC_SAMPLE_UPDATE_PERIOD_MS = 500; private static final long VSYNC_SAMPLE_UPDATE_PERIOD_MS = 500;
/** /**
...@@ -65,7 +90,7 @@ public final class VideoFrameReleaseHelper { ...@@ -65,7 +90,7 @@ public final class VideoFrameReleaseHelper {
*/ */
private static final long VSYNC_OFFSET_PERCENTAGE = 80; private static final long VSYNC_OFFSET_PERCENTAGE = 80;
private final FixedFrameRateEstimator fixedFrameRateEstimator; private final FixedFrameRateEstimator frameRateEstimator;
@Nullable private final WindowManager windowManager; @Nullable private final WindowManager windowManager;
@Nullable private final VSyncSampler vsyncSampler; @Nullable private final VSyncSampler vsyncSampler;
@Nullable private final DefaultDisplayListener displayListener; @Nullable private final DefaultDisplayListener displayListener;
...@@ -73,9 +98,18 @@ public final class VideoFrameReleaseHelper { ...@@ -73,9 +98,18 @@ public final class VideoFrameReleaseHelper {
private boolean started; private boolean started;
@Nullable private Surface surface; @Nullable private Surface surface;
private float surfaceFrameRate; /** The media frame rate specified in the {@link Format}. */
private float formatFrameRate; private float formatFrameRate;
private double playbackSpeed; /**
* The media frame rate used to calculate the playback frame rate of the {@link Surface}. This may
* be different to {@link #formatFrameRate} if {@link #formatFrameRate} is unspecified or
* inaccurate.
*/
private float surfaceMediaFrameRate;
/** The playback frame rate set on the {@link Surface}. */
private float surfacePlaybackFrameRate;
private float playbackSpeed;
private long vsyncDurationNs; private long vsyncDurationNs;
private long vsyncOffsetNs; private long vsyncOffsetNs;
...@@ -92,7 +126,7 @@ public final class VideoFrameReleaseHelper { ...@@ -92,7 +126,7 @@ public final class VideoFrameReleaseHelper {
* @param context A context from which information about the default display can be retrieved. * @param context A context from which information about the default display can be retrieved.
*/ */
public VideoFrameReleaseHelper(@Nullable Context context) { public VideoFrameReleaseHelper(@Nullable Context context) {
fixedFrameRateEstimator = new FixedFrameRateEstimator(); frameRateEstimator = new FixedFrameRateEstimator();
if (context != null) { if (context != null) {
context = context.getApplicationContext(); context = context.getApplicationContext();
windowManager = (WindowManager) context.getSystemService(Context.WINDOW_SERVICE); windowManager = (WindowManager) context.getSystemService(Context.WINDOW_SERVICE);
...@@ -116,7 +150,6 @@ public final class VideoFrameReleaseHelper { ...@@ -116,7 +150,6 @@ public final class VideoFrameReleaseHelper {
/** Called when the renderer is enabled. */ /** Called when the renderer is enabled. */
@TargetApi(17) // displayListener is null if Util.SDK_INT < 17. @TargetApi(17) // displayListener is null if Util.SDK_INT < 17.
public void onEnabled() { public void onEnabled() {
fixedFrameRateEstimator.reset();
if (windowManager != null) { if (windowManager != null) {
checkNotNull(vsyncSampler).addObserver(); checkNotNull(vsyncSampler).addObserver();
if (displayListener != null) { if (displayListener != null) {
...@@ -130,7 +163,7 @@ public final class VideoFrameReleaseHelper { ...@@ -130,7 +163,7 @@ public final class VideoFrameReleaseHelper {
public void onStarted() { public void onStarted() {
started = true; started = true;
resetAdjustment(); resetAdjustment();
updateSurfaceFrameRate(/* isNewSurface= */ false); updateSurfacePlaybackFrameRate(/* isNewSurface= */ false);
} }
/** /**
...@@ -148,7 +181,7 @@ public final class VideoFrameReleaseHelper { ...@@ -148,7 +181,7 @@ public final class VideoFrameReleaseHelper {
} }
clearSurfaceFrameRate(); clearSurfaceFrameRate();
this.surface = surface; this.surface = surface;
updateSurfaceFrameRate(/* isNewSurface= */ true); updateSurfacePlaybackFrameRate(/* isNewSurface= */ true);
} }
/** Called when the renderer's position is reset. */ /** Called when the renderer's position is reset. */
...@@ -162,10 +195,10 @@ public final class VideoFrameReleaseHelper { ...@@ -162,10 +195,10 @@ public final class VideoFrameReleaseHelper {
* *
* @param playbackSpeed The player's speed. * @param playbackSpeed The player's speed.
*/ */
public void onPlaybackSpeed(double playbackSpeed) { public void onPlaybackSpeed(float playbackSpeed) {
this.playbackSpeed = playbackSpeed; this.playbackSpeed = playbackSpeed;
resetAdjustment(); resetAdjustment();
updateSurfaceFrameRate(/* isNewSurface= */ false); updateSurfacePlaybackFrameRate(/* isNewSurface= */ false);
} }
/** /**
...@@ -175,8 +208,8 @@ public final class VideoFrameReleaseHelper { ...@@ -175,8 +208,8 @@ public final class VideoFrameReleaseHelper {
*/ */
public void onFormatChanged(float formatFrameRate) { public void onFormatChanged(float formatFrameRate) {
this.formatFrameRate = formatFrameRate; this.formatFrameRate = formatFrameRate;
fixedFrameRateEstimator.onFormatChanged(formatFrameRate); frameRateEstimator.reset();
updateSurfaceFrameRate(/* isNewSurface= */ false); updateSurfaceMediaFrameRate();
} }
/** /**
...@@ -189,8 +222,9 @@ public final class VideoFrameReleaseHelper { ...@@ -189,8 +222,9 @@ public final class VideoFrameReleaseHelper {
lastAdjustedFrameIndex = pendingLastAdjustedFrameIndex; lastAdjustedFrameIndex = pendingLastAdjustedFrameIndex;
lastAdjustedReleaseTimeNs = pendingLastAdjustedReleaseTimeNs; lastAdjustedReleaseTimeNs = pendingLastAdjustedReleaseTimeNs;
} }
fixedFrameRateEstimator.onNextFrame(framePresentationTimeUs * 1000);
frameIndex++; frameIndex++;
frameRateEstimator.onNextFrame(framePresentationTimeUs * 1000);
updateSurfaceMediaFrameRate();
} }
/** Called when the renderer is stopped. */ /** Called when the renderer is stopped. */
...@@ -230,8 +264,8 @@ public final class VideoFrameReleaseHelper { ...@@ -230,8 +264,8 @@ public final class VideoFrameReleaseHelper {
// Until we know better, the adjustment will be a no-op. // Until we know better, the adjustment will be a no-op.
long adjustedReleaseTimeNs = releaseTimeNs; long adjustedReleaseTimeNs = releaseTimeNs;
if (lastAdjustedFrameIndex != C.INDEX_UNSET && fixedFrameRateEstimator.isSynced()) { if (lastAdjustedFrameIndex != C.INDEX_UNSET && frameRateEstimator.isSynced()) {
long frameDurationNs = fixedFrameRateEstimator.getFrameDurationNs(); long frameDurationNs = frameRateEstimator.getFrameDurationNs();
long candidateAdjustedReleaseTimeNs = long candidateAdjustedReleaseTimeNs =
lastAdjustedReleaseTimeNs lastAdjustedReleaseTimeNs
+ (long) ((frameDurationNs * (frameIndex - lastAdjustedFrameIndex)) / playbackSpeed); + (long) ((frameDurationNs * (frameIndex - lastAdjustedFrameIndex)) / playbackSpeed);
...@@ -271,36 +305,78 @@ public final class VideoFrameReleaseHelper { ...@@ -271,36 +305,78 @@ public final class VideoFrameReleaseHelper {
// Surface frame rate adjustment. // Surface frame rate adjustment.
/** /**
* Updates the frame-rate of the current {@link #surface} based on the renderer operating rate, * Updates the media frame rate that's used to calculate the playback frame rate of the current
* frame-rate of the content, and whether the renderer is started. * {@link #surface}. If the frame rate is updated then {@link #updateSurfacePlaybackFrameRate} is
* called to update the surface.
*/
private void updateSurfaceMediaFrameRate() {
if (Util.SDK_INT < 30 || surface == null) {
return;
}
float candidateFrameRate =
frameRateEstimator.isSynced() ? frameRateEstimator.getFrameRate() : formatFrameRate;
if (candidateFrameRate == surfaceMediaFrameRate) {
return;
}
// The candidate is different to the current surface media frame rate. Decide whether to update
// the surface media frame rate.
boolean shouldUpdate;
if (candidateFrameRate != Format.NO_VALUE && surfaceMediaFrameRate != Format.NO_VALUE) {
boolean candidateIsHighConfidence =
frameRateEstimator.isSynced()
&& frameRateEstimator.getMatchingFrameDurationSumNs()
>= MINIMUM_MATCHING_FRAME_DURATION_FOR_HIGH_CONFIDENCE_NS;
float minimumChangeForUpdate =
candidateIsHighConfidence
? MINIMUM_MEDIA_FRAME_RATE_CHANGE_FOR_UPDATE_HIGH_CONFIDENCE
: MINIMUM_MEDIA_FRAME_RATE_CHANGE_FOR_UPDATE_LOW_CONFIDENCE;
shouldUpdate = Math.abs(candidateFrameRate - surfaceMediaFrameRate) >= minimumChangeForUpdate;
} else if (candidateFrameRate != Format.NO_VALUE) {
shouldUpdate = true;
} else {
shouldUpdate =
frameRateEstimator.getFramesWithoutSyncCount()
>= MINIMUM_FRAMES_WITHOUT_SYNC_TO_CLEAR_SURFACE_FRAME_RATE;
}
if (shouldUpdate) {
surfaceMediaFrameRate = candidateFrameRate;
updateSurfacePlaybackFrameRate(/* isNewSurface= */ false);
}
}
/**
* Updates the playback frame rate of the current {@link #surface} based on the playback speed,
* frame rate of the content, and whether the renderer is started.
* *
* @param isNewSurface Whether the current {@link #surface} is new. * @param isNewSurface Whether the current {@link #surface} is new.
*/ */
private void updateSurfaceFrameRate(boolean isNewSurface) { private void updateSurfacePlaybackFrameRate(boolean isNewSurface) {
if (Util.SDK_INT < 30 || surface == null) { if (Util.SDK_INT < 30 || surface == null) {
return; return;
} }
float surfaceFrameRate = 0; float surfacePlaybackFrameRate = 0;
// TODO: Hook up fixedFrameRateEstimator. if (started && surfaceMediaFrameRate != Format.NO_VALUE) {
if (started && formatFrameRate != Format.NO_VALUE) { surfacePlaybackFrameRate = surfaceMediaFrameRate * playbackSpeed;
surfaceFrameRate = (float) (formatFrameRate * playbackSpeed);
} }
// We always set the frame-rate if we have a new surface, since we have no way of knowing what // We always set the frame-rate if we have a new surface, since we have no way of knowing what
// it might have been set to previously. // it might have been set to previously.
if (this.surfaceFrameRate == surfaceFrameRate && !isNewSurface) { if (!isNewSurface && this.surfacePlaybackFrameRate == surfacePlaybackFrameRate) {
return; return;
} }
this.surfaceFrameRate = surfaceFrameRate; this.surfacePlaybackFrameRate = surfacePlaybackFrameRate;
setSurfaceFrameRateV30(surface, surfaceFrameRate); setSurfaceFrameRateV30(surface, surfacePlaybackFrameRate);
} }
/** Clears the frame-rate of the current {@link #surface}. */ /** Clears the frame-rate of the current {@link #surface}. */
private void clearSurfaceFrameRate() { private void clearSurfaceFrameRate() {
if (Util.SDK_INT < 30 || surface == null || surfaceFrameRate == 0) { if (Util.SDK_INT < 30 || surface == null || surfacePlaybackFrameRate == 0) {
return; return;
} }
surfaceFrameRate = 0; surfacePlaybackFrameRate = 0;
setSurfaceFrameRateV30(surface, /* frameRate= */ 0); setSurfaceFrameRateV30(surface, /* frameRate= */ 0);
} }
......
...@@ -202,93 +202,6 @@ public final class FixedFrameRateEstimatorTest { ...@@ -202,93 +202,6 @@ public final class FixedFrameRateEstimatorTest {
} }
} }
@Test
public void newFixedFrameRate_withFormatFrameRateChange_resyncs() {
long frameDurationNs = 33_333_333;
float frameRate = (float) C.NANOS_PER_SECOND / frameDurationNs;
FixedFrameRateEstimator estimator = new FixedFrameRateEstimator();
estimator.onFormatChanged(frameRate);
long framePresentationTimestampNs = 0;
estimator.onNextFrame(framePresentationTimestampNs);
for (int i = 0; i < CONSECUTIVE_MATCHING_FRAME_DURATIONS_FOR_SYNC; i++) {
framePresentationTimestampNs += frameDurationNs;
estimator.onNextFrame(framePresentationTimestampNs);
}
assertThat(estimator.isSynced()).isTrue();
assertThat(estimator.getFrameDurationNs()).isEqualTo(frameDurationNs);
// Frames durations are halved from this point.
long halfFrameDuration = frameDurationNs * 2;
float doubleFrameRate = (float) C.NANOS_PER_SECOND / halfFrameDuration;
estimator.onFormatChanged(doubleFrameRate);
// Format frame rate change should cause immediate sync loss.
assertThat(estimator.isSynced()).isFalse();
assertThat(estimator.getFrameDurationNs()).isEqualTo(C.TIME_UNSET);
// Frames with consistent durations, working toward establishing new sync.
for (int i = 0; i < CONSECUTIVE_MATCHING_FRAME_DURATIONS_FOR_SYNC; i++) {
framePresentationTimestampNs += halfFrameDuration;
estimator.onNextFrame(framePresentationTimestampNs);
assertThat(estimator.isSynced()).isFalse();
assertThat(estimator.getFrameDurationNs()).isEqualTo(C.TIME_UNSET);
}
// This frame should establish sync.
framePresentationTimestampNs += halfFrameDuration;
estimator.onNextFrame(framePresentationTimestampNs);
assertThat(estimator.isSynced()).isTrue();
assertThat(estimator.getFrameDurationNs()).isEqualTo(halfFrameDuration);
}
@Test
public void smallFrameRateChange_withoutFormatFrameRateChange_keepsSyncAndAdjustsEstimate() {
long frameDurationNs = 33_333_333; // 30 fps
float roundedFrameRate = 30;
FixedFrameRateEstimator estimator = new FixedFrameRateEstimator();
estimator.onFormatChanged(roundedFrameRate);
long framePresentationTimestampNs = 0;
estimator.onNextFrame(framePresentationTimestampNs);
for (int i = 0; i < CONSECUTIVE_MATCHING_FRAME_DURATIONS_FOR_SYNC; i++) {
framePresentationTimestampNs += frameDurationNs;
estimator.onNextFrame(framePresentationTimestampNs);
}
assertThat(estimator.isSynced()).isTrue();
assertThat(estimator.getFrameDurationNs()).isEqualTo(frameDurationNs);
long newFrameDurationNs = 33_366_667; // 30 * (1000/1001) = 29.97 fps
estimator.onFormatChanged(roundedFrameRate); // Format frame rate is unchanged.
// Previous estimate should remain valid for now because neither format specified a duration.
assertThat(estimator.isSynced()).isTrue();
assertThat(estimator.getFrameDurationNs()).isEqualTo(frameDurationNs);
// The estimate should start moving toward the new frame duration. If should not lose sync
// because the change in frame rate is very small.
for (int i = 0; i < CONSECUTIVE_MATCHING_FRAME_DURATIONS_FOR_SYNC - 1; i++) {
framePresentationTimestampNs += newFrameDurationNs;
estimator.onNextFrame(framePresentationTimestampNs);
assertThat(estimator.isSynced()).isTrue();
assertThat(estimator.getFrameDurationNs()).isGreaterThan(frameDurationNs);
assertThat(estimator.getFrameDurationNs()).isLessThan(newFrameDurationNs);
}
framePresentationTimestampNs += newFrameDurationNs;
estimator.onNextFrame(framePresentationTimestampNs);
// Frames with the previous frame duration should now be excluded from the estimate, so the
// estimate should become exact.
assertThat(estimator.isSynced()).isTrue();
assertThat(estimator.getFrameDurationNs()).isEqualTo(newFrameDurationNs);
}
private static final long getNsWithMsPrecision(long presentationTimeNs) { private static final long getNsWithMsPrecision(long presentationTimeNs) {
return (presentationTimeNs / 1000000) * 1000000; return (presentationTimeNs / 1000000) * 1000000;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment