Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
136523002:
WebMediaPlayerAndroid - handling unseekable durations (Closed)
|
Created:
6 years, 11 months ago by amogh.bihani Modified:
6 years, 11 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCurrently duration is simply returned or changed without checking whether the stream is seekable or not. MediaSourcePLayer deems a stream unseekable if the duration is more than 2^31 msec. I'm adding that check here as per the TODO.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245508
Patch Set 1 #
Total comments: 8
Patch Set 2 : changes made according to the review #
Total comments: 7
Patch Set 3 : adding kInifiniteDuration check, removing 2^31 limiting check from onDurationChanged #
Total comments: 2
Patch Set 4 : dividing by 1000 to convert from millisecond to second #
Total comments: 2
Patch Set 5 : changing 1000 to 1000.0 #Patch Set 6 : removing nit #Messages
Total messages: 17 (0 generated)
PTAL https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:505: // TODO(hclam): If this stream is not seekable this should return 0. This is handled in this patch automatically https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:623: // considers unseekable, including kInfiniteDuration(). kInfiniteDuration is 2^63, so this is automatically handled
https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:506: return duration(); This does not see right to me. I believe this should be std::min(std::numeric_limits<int32>max(), duration()) instead so that the seekable range is never too large. This allow Blink to prevent invalid seek values from ever being passed to seek(). https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:389: if (new_seek_time >= This should not be needed if maxTimeSeekable() never returns a value greater than std::numeric_limits<int32>max(). https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:629: if (duration >= This does not seem right. The clip duration and the seekable range are 2 different concepts. I don't think it is correct to prevent duration updates just because the seekable range is smaller than the duration. The duration update should be allowed to proceed and as long as maxTimeSeekable() is returning the right information, Blink should prevent seeks from outside the seekable range. If it isn't then we probably need to make a change to the Blink code instead.
Removing myself from reviewer list.
Thanks for the review :) I have made the changes as suggested by you. Please take a look.
https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:506: return duration(); On 2014/01/13 16:32:07, acolwell wrote: > This does not see right to me. I believe this should be > std::min(std::numeric_limits<int32>max(), duration()) instead so that the > seekable range is never too large. This allow Blink to prevent invalid seek > values from ever being passed to seek(). Done. https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:389: if (new_seek_time >= On 2014/01/13 16:32:07, acolwell wrote: > This should not be needed if maxTimeSeekable() never returns a value greater > than std::numeric_limits<int32>max(). Done. https://codereview.chromium.org/136523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:629: if (duration >= On 2014/01/13 16:32:07, acolwell wrote: > This does not seem right. The clip duration and the seekable range are 2 > different concepts. I don't think it is correct to prevent duration updates just > because the seekable range is smaller than the duration. The duration update > should be allowed to proceed and as long as maxTimeSeekable() is returning the > right information, Blink should prevent seeks from outside the seekable range. > If it isn't then we probably need to make a change to the Blink code instead. Done.
Just a few more comments. Also please add tests that verify the new code paths behave as expected. https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:465: // considers unseekable, including kInfiniteDuration(). This comment should not be removed since this code still isn't handling kInfiniteDuration(). If you add the following then removing this comment is ok. if (duration_ == media::kInfiniteDuration()) return std::numeric_limits<double>::infinity(); https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:501: You also need the following to properly handle clips with an infinite duration. if (duration() == std::numeric_limits<double>::infinity()) return 0; https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:826: if (duration >= This prevents Blink from being notified of the new duration. I think these changes should be removed. The seekable range logic should prevent the seeking to times outside the range.
On 2014/01/14 16:36:40, acolwell wrote: > Just a few more comments. Also please add tests that verify the new code paths > behave as expected. > Sure... test case coming in next patchset :)
https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:465: // considers unseekable, including kInfiniteDuration(). On 2014/01/14 16:36:40, acolwell wrote: > This comment should not be removed since this code still isn't handling > kInfiniteDuration(). > > If you add the following then removing this comment is ok. > > if (duration_ == media::kInfiniteDuration()) > return std::numeric_limits<double>::infinity(); initially when i had stopped duration to be more than 2^31, this was being hadled as kInfiniteDuration is 2^63. Forgot to change this when i removed it in second patchset. Ill do the change as suggested buy you. https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:465: // considers unseekable, including kInfiniteDuration(). On 2014/01/14 16:36:40, acolwell wrote: > This comment should not be removed since this code still isn't handling > kInfiniteDuration(). > > If you add the following then removing this comment is ok. > > if (duration_ == media::kInfiniteDuration()) > return std::numeric_limits<double>::infinity(); Done. https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:501: On 2014/01/14 16:36:40, acolwell wrote: > You also need the following to properly handle clips with an infinite duration. > > if (duration() == std::numeric_limits<double>::infinity()) > return 0; Done. https://codereview.chromium.org/136523002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:826: if (duration >= On 2014/01/14 16:36:40, acolwell wrote: > This prevents Blink from being notified of the new duration. I think these > changes should be removed. The seekable range logic should prevent the seeking > to times outside the range. Done.
https://codereview.chromium.org/136523002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:508: return std::min(std::numeric_limits<int32>max(), duration()); Drive-by comment: The Java media player is limited to maximum seekable duration of std::numeric_limits<int32>::max() interpreted in milliseconds. This line is incorrect: It is claiming seekability to 1000x the actual range of the underlying Android media player.
https://codereview.chromium.org/136523002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:508: return std::min(std::numeric_limits<int32>max(), duration()); On 2014/01/15 20:47:29, wolenetz wrote: > Drive-by comment: The Java media player is limited to maximum seekable duration > of std::numeric_limits<int32>::max() interpreted in milliseconds. This line is > incorrect: It is claiming seekability to 1000x the actual range of the > underlying Android media player. Done.
acolwell@ It seems that no one has made testcase for webmediaplayer_android. I did make unittests but to implement them I need to have mock_webMediaPlayerClient mock_rendererMediaPlayerManager, mock_WebMediaPlayerDelegate mock_webFrame of these only mock_webFrame is available, others are not available as of now... so it would be quite difficult to have unittest for these changes. :(
lgtm % nit https://codereview.chromium.org/136523002/diff/180001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/180001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:508: return std::min(std::numeric_limits<int32>max() / 1000, duration()); nit:s/1000/1000.0/ so that we get the full range and not just the integer number of seconds.
Thanks for the review :) https://codereview.chromium.org/136523002/diff/180001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/136523002/diff/180001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:508: return std::min(std::numeric_limits<int32>max() / 1000, duration()); On 2014/01/16 18:17:47, acolwell wrote: > nit:s/1000/1000.0/ so that we get the full range and not just the integer number > of seconds. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/136523002/240001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/136523002/520001
Message was sent while issue was closed.
Change committed as 245508 |
