Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
505233002:
Add UMA to study the video types for mobile (Closed)
|
Created:
6 years, 3 months ago by qinmin Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, avayvod+watch_chromium.org, asvitkine+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA to study the video types for mobile
Before we work on transfering from MediaPlayer to MediaCodec, we want to get some statistics on the current mobile video types to see the impacts.
We want to know how many video are HLS(which will still use MediaPlayer) and how many videos are using MediaSource(which already uses MediaCodec)
BUG=401795
Committed: https://crrev.com/543663f968c044c1c417187936696e5844c7c4ef
Cr-Commit-Position: refs/heads/master@{#292756}
Patch Set 1 #
Total comments: 14
Patch Set 2 : addressing comments #
Total comments: 4
Patch Set 3 : addressing comments #
Total comments: 11
Patch Set 4 : reduce the number of UMA to 1 variable #Patch Set 5 : reuse desktop UMAs #
Total comments: 4
Patch Set 6 : addressing scherkus's comments #
Total comments: 2
Patch Set 7 : Fix logic when checking HLS #
Total comments: 2
Messages
Total messages: 30 (1 generated)
qinmin@chromium.org changed reviewers: + isherman@chromium.org, xhwang@chromium.org
PTAL
https://codereview.chromium.org/505233002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:111: UMA_HISTOGRAM_BOOLEAN("MobileVideo.IsVideoUrlHLSVideo", false); nit: Please write this as bool is_hls_video = EndsWith(path, ".m3u8", true) || EndsWith(path, ".m3u", true); UMA_HISTOGRAM_BOOLEAN("MobileVideo.IsVideoUrlHLSVideo", is_hls_video); https://codereview.chromium.org/505233002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:118: UMA_HISTOGRAM_BOOLEAN("MobileVideo.IsRegularVideoUrl", false); nit: It's generally best to avoid repeating UMA_HISTOGRAM macros for a single histogram, to reduce the likelihood of having a typo in just one of the code paths. I'd suggest creating a tiny wrapper function to which you pass |true| or |false| to be emitted to the histogram. https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12252: +<histogram name="MobileVideo.IsRegularVideoUrl" enum="BooleanEnabled"> What do you think of naming this histogram something more like "Media.Video.Mobile.IsRegularVideoUrl"? The UMA dashboard is a little easier to use when there aren't a bunch of tiny groups, but rather a smaller number of bigger top-level groups. https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12252: +<histogram name="MobileVideo.IsRegularVideoUrl" enum="BooleanEnabled"> Please use a custom enum, rather than "BooleanEnabled". https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12255: + Android: Records whether a video is a regular video url rather than is nit: "rather than is" -> "rather than one that is" https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12260: +<histogram name="MobileVideo.IsVideoUrlHLSVideo" enum="BooleanEnabled"> nit: I'd prefer that you spell out the acronym in the name. How about "IsHttpLiveStreamingVideo", or something like that? https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12263: + Android: Whether a regular video url is an HLS(Http live streaming) video. nit: "HLS(Http live streaming)" -> "HLS (http live streaming)"
https://codereview.chromium.org/505233002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:111: UMA_HISTOGRAM_BOOLEAN("MobileVideo.IsVideoUrlHLSVideo", false); On 2014/08/26 19:54:28, Ilya Sherman wrote: > nit: Please write this as > > bool is_hls_video = EndsWith(path, ".m3u8", true) || EndsWith(path, ".m3u", > true); > UMA_HISTOGRAM_BOOLEAN("MobileVideo.IsVideoUrlHLSVideo", is_hls_video); Done. https://codereview.chromium.org/505233002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:118: UMA_HISTOGRAM_BOOLEAN("MobileVideo.IsRegularVideoUrl", false); On 2014/08/26 19:54:29, Ilya Sherman wrote: > nit: It's generally best to avoid repeating UMA_HISTOGRAM macros for a single > histogram, to reduce the likelihood of having a typo in just one of the code > paths. I'd suggest creating a tiny wrapper function to which you pass |true| or > |false| to be emitted to the histogram. Done. https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12252: +<histogram name="MobileVideo.IsRegularVideoUrl" enum="BooleanEnabled"> On 2014/08/26 19:54:29, Ilya Sherman wrote: > Please use a custom enum, rather than "BooleanEnabled". Done. added MediaType Enum https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12252: +<histogram name="MobileVideo.IsRegularVideoUrl" enum="BooleanEnabled"> Changed this to Media.Mobile.MediaType and updated the description On 2014/08/26 19:54:29, Ilya Sherman wrote: > What do you think of naming this histogram something more like > "Media.Video.Mobile.IsRegularVideoUrl"? The UMA dashboard is a little easier to > use when there aren't a bunch of tiny groups, but rather a smaller number of > bigger top-level groups. https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12255: + Android: Records whether a video is a regular video url rather than is On 2014/08/26 19:54:29, Ilya Sherman wrote: > nit: "rather than is" -> "rather than one that is" Done. https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12260: +<histogram name="MobileVideo.IsVideoUrlHLSVideo" enum="BooleanEnabled"> On 2014/08/26 19:54:29, Ilya Sherman wrote: > nit: I'd prefer that you spell out the acronym in the name. How about > "IsHttpLiveStreamingVideo", or something like that? Done. https://codereview.chromium.org/505233002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12263: + Android: Whether a regular video url is an HLS(Http live streaming) video. On 2014/08/26 19:54:29, Ilya Sherman wrote: > nit: "HLS(Http live streaming)" -> "HLS (http live streaming)" Done.
https://codereview.chromium.org/505233002/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:51: UMA_HISTOGRAM_ENUMERATION("Media.Mobile.MediaType", Why not use UMA_HISTOGRAM_BOOLEAN? https://codereview.chromium.org/505233002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/505233002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11313: +<histogram name="Media.Mobile.IsHttpLiveStreamingMedia" enum="BooleanEnabled"> Please use a more specific enum. (Note: This only requires a histograms.xml change -- no C++ change needed.)
https://codereview.chromium.org/505233002/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:51: UMA_HISTOGRAM_ENUMERATION("Media.Mobile.MediaType", On 2014/08/27 00:31:27, Ilya Sherman wrote: > Why not use UMA_HISTOGRAM_BOOLEAN? Done. https://codereview.chromium.org/505233002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/505233002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11313: +<histogram name="Media.Mobile.IsHttpLiveStreamingMedia" enum="BooleanEnabled"> On 2014/08/27 00:31:27, Ilya Sherman wrote: > Please use a more specific enum. (Note: This only requires a histograms.xml > change -- no C++ change needed.) Done.
LGTM % comment. https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:45: const int kMaxMediaTypes = 2; nit: This is now unused. https://codereview.chromium.org/505233002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/505233002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:44965: + <int value="0" label="File Type"/> Are you sure that non-HLS videos will always be of file type? Since you're currently logging these two histograms as boolean histograms, I would have expected to see enums that each had labels along the lines of "foo" vs. "not foo".
The CL looks good. I just have some crazy nits for discussion. https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:50: void RecordMobileMediaType(bool is_media_url) { nit: Won't it be easier if you just report MediaPlayerHostMsg_Initialize_Type to UMA (instead of using a boolean, which is harder to understand)? https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:51: UMA_HISTOGRAM_BOOLEAN("Media.Mobile.MediaType", is_media_url); nit: Since this only applies to Chrome for Android (not Chrome for IOS), I wonder whether "Mobile" is the correct term. How about s/Mobile/Android? https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:118: is_hls_media); nit: Since both UMAs are pretty small (having only two entries), probably we could have just one UMA for all, which has: URL_NON_HLS URL_HLS MEDIA_SOURCE Then we have one central place to show the media type on Android. WDYT?
https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:45: const int kMaxMediaTypes = 2; changed to 3 as we combine the 3 types. On 2014/08/27 22:14:37, Ilya Sherman wrote: > nit: This is now unused. https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:50: void RecordMobileMediaType(bool is_media_url) { On 2014/08/27 23:02:21, xhwang wrote: > nit: Won't it be easier if you just report MediaPlayerHostMsg_Initialize_Type to > UMA (instead of using a boolean, which is harder to understand)? Done. https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:51: UMA_HISTOGRAM_BOOLEAN("Media.Mobile.MediaType", is_media_url); On 2014/08/27 23:02:21, xhwang wrote: > nit: Since this only applies to Chrome for Android (not Chrome for IOS), I > wonder whether "Mobile" is the correct term. How about s/Mobile/Android? Done. https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:118: is_hls_media); On 2014/08/27 23:02:21, xhwang wrote: > nit: > > Since both UMAs are pretty small (having only two entries), probably we could > have just one UMA for all, which has: > URL_NON_HLS > URL_HLS > MEDIA_SOURCE > > Then we have one central place to show the media type on Android. WDYT? Done. https://codereview.chromium.org/505233002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/505233002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:44965: + <int value="0" label="File Type"/> On 2014/08/27 22:14:37, Ilya Sherman wrote: > Are you sure that non-HLS videos will always be of file type? Since you're > currently logging these two histograms as boolean histograms, I would have > expected to see enums that each had labels along the lines of "foo" vs. "not > foo". Done.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:118: is_hls_media); On 2014/08/28 00:30:09, qinmin wrote: > On 2014/08/27 23:02:21, xhwang wrote: > > nit: > > > > Since both UMAs are pretty small (having only two entries), probably we could > > have just one UMA for all, which has: > > URL_NON_HLS > > URL_HLS > > MEDIA_SOURCE > > > > Then we have one central place to show the media type on Android. WDYT? > > Done. drive-by: Would it make more sense to have a "container" UMA or add HLS to an existing one? I think we already have an MSE vs. normal UMA on desktop.
On 2014/08/28 00:59:14, ddorwin wrote: > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > File content/browser/media/android/browser_media_player_manager.cc (right): > > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > content/browser/media/android/browser_media_player_manager.cc:118: > is_hls_media); > On 2014/08/28 00:30:09, qinmin wrote: > > On 2014/08/27 23:02:21, xhwang wrote: > > > nit: > > > > > > Since both UMAs are pretty small (having only two entries), probably we > could > > > have just one UMA for all, which has: > > > URL_NON_HLS > > > URL_HLS > > > MEDIA_SOURCE > > > > > > Then we have one central place to show the media type on Android. WDYT? > > > > Done. > > drive-by: > Would it make more sense to have a "container" UMA or add HLS to an existing > one? I think we already have an MSE vs. normal UMA on desktop. Where is the desktop UMA for non MSE and MSE? I didn't find it in histograms.xml
ddorwin@chromium.org changed reviewers: - ddorwin@chromium.org, isherman@chromium.org
ddorwin@chromium.org changed reviewers: + isherman@chromium.org
On 2014/08/28 01:08:54, qinmin wrote: > On 2014/08/28 00:59:14, ddorwin wrote: > > > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > > File content/browser/media/android/browser_media_player_manager.cc (right): > > > > > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > > content/browser/media/android/browser_media_player_manager.cc:118: > > is_hls_media); > > On 2014/08/28 00:30:09, qinmin wrote: > > > On 2014/08/27 23:02:21, xhwang wrote: > > > > nit: > > > > > > > > Since both UMAs are pretty small (having only two entries), probably we > > could > > > > have just one UMA for all, which has: > > > > URL_NON_HLS > > > > URL_HLS > > > > MEDIA_SOURCE > > > > > > > > Then we have one central place to show the media type on Android. WDYT? > > > > > > Done. > > > > drive-by: > > Would it make more sense to have a "container" UMA or add HLS to an existing > > one? I think we already have an MSE vs. normal UMA on desktop. > > Where is the desktop UMA for non MSE and MSE? I didn't find it in histograms.xml Media.MSE.Playback, which is a bit confusing: Success: Number of MSE playback Failure: Number of non-MSE (SRC) playback. I don't know whether this is exactly appropriate for the container, but we have Media.DetectedContainer (type is MediaContainers).
On 2014/08/28 01:20:32, ddorwin wrote: > On 2014/08/28 01:08:54, qinmin wrote: > > On 2014/08/28 00:59:14, ddorwin wrote: > > > > > > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > > > File content/browser/media/android/browser_media_player_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > > > content/browser/media/android/browser_media_player_manager.cc:118: > > > is_hls_media); > > > On 2014/08/28 00:30:09, qinmin wrote: > > > > On 2014/08/27 23:02:21, xhwang wrote: > > > > > nit: > > > > > > > > > > Since both UMAs are pretty small (having only two entries), probably we > > > could > > > > > have just one UMA for all, which has: > > > > > URL_NON_HLS > > > > > URL_HLS > > > > > MEDIA_SOURCE > > > > > > > > > > Then we have one central place to show the media type on Android. WDYT? > > > > > > > > Done. > > > > > > drive-by: > > > Would it make more sense to have a "container" UMA or add HLS to an existing > > > one? I think we already have an MSE vs. normal UMA on desktop. > > > > Where is the desktop UMA for non MSE and MSE? I didn't find it in > histograms.xml > > Media.MSE.Playback, which is a bit confusing: > Success: Number of MSE playback > Failure: Number of non-MSE (SRC) playback. > > I don't know whether this is exactly appropriate for the container, but we have > Media.DetectedContainer (type is MediaContainers). I think actually that's a good idea. We can reuse the Media.MSE.Playback for android. For containers, we can just use UNKOWN and HLS for android.
On 2014/08/28 01:35:50, qinmin wrote: > On 2014/08/28 01:20:32, ddorwin wrote: > > On 2014/08/28 01:08:54, qinmin wrote: > > > On 2014/08/28 00:59:14, ddorwin wrote: > > > > > > > > > > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > > > > File content/browser/media/android/browser_media_player_manager.cc > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/505233002/diff/40001/content/browser/media/an... > > > > content/browser/media/android/browser_media_player_manager.cc:118: > > > > is_hls_media); > > > > On 2014/08/28 00:30:09, qinmin wrote: > > > > > On 2014/08/27 23:02:21, xhwang wrote: > > > > > > nit: > > > > > > > > > > > > Since both UMAs are pretty small (having only two entries), probably > we > > > > could > > > > > > have just one UMA for all, which has: > > > > > > URL_NON_HLS > > > > > > URL_HLS > > > > > > MEDIA_SOURCE > > > > > > > > > > > > Then we have one central place to show the media type on Android. > WDYT? > > > > > > > > > > Done. > > > > > > > > drive-by: > > > > Would it make more sense to have a "container" UMA or add HLS to an > existing > > > > one? I think we already have an MSE vs. normal UMA on desktop. > > > > > > Where is the desktop UMA for non MSE and MSE? I didn't find it in > > histograms.xml > > > > Media.MSE.Playback, which is a bit confusing: > > Success: Number of MSE playback > > Failure: Number of non-MSE (SRC) playback. > > > > I don't know whether this is exactly appropriate for the container, but we > have > > Media.DetectedContainer (type is MediaContainers). > > I think actually that's a good idea. We can reuse the Media.MSE.Playback for > android. For containers, we can just use UNKOWN and HLS for android. The current Media.MSE.Playback is pretty confusing. But I agree it would be better to have consistent UMAs for desktop and Android. If you agree, let's do the desktop way and refactor that later together.
Ok, Modified the change to reuse desktop UMAs > The current Media.MSE.Playback is pretty confusing. But I agree it would be > better to have consistent UMAs for desktop and Android. If you agree, let's do > the desktop way and refactor that later together.
scherkus@chromium.org changed reviewers: + scherkus@chromium.org
sorry to pile on here but I've got some questions... https://codereview.chromium.org/505233002/diff/80001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/80001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:81: UMA_HISTOGRAM_BOOLEAN( any reason why we don't log these UMAs when WebMediaPlayerAndroid::load() is called, similar to WebMediaPlayerImpl? https://codereview.chromium.org/505233002/diff/80001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:122: "Media.DetectedContainer", media_type, HTTP_LIVE_STREAM_TYPE + 1); to be honest I don't think it's worth reusing this particular histogram as you're: a) redefining a subset of the enum it uses b) detecting the type based on URL suffix rather than how Media.DetectedContainer currently detects the type by inspecting bytes considering this is Android specific, I'd by fine having a histogram for answering the question "Is this an HLS stream or not?" also same question as above w.r.t. doing this inside of WebMediaPlayerAndroid::load() -- we already have a IsHLSStream() method there :)
https://codereview.chromium.org/505233002/diff/80001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/505233002/diff/80001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:81: UMA_HISTOGRAM_BOOLEAN( On 2014/08/28 19:07:28, scherkus wrote: > any reason why we don't log these UMAs when WebMediaPlayerAndroid::load() is > called, similar to WebMediaPlayerImpl? Done. Moved it to WMPA. https://codereview.chromium.org/505233002/diff/80001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:122: "Media.DetectedContainer", media_type, HTTP_LIVE_STREAM_TYPE + 1); On 2014/08/28 19:07:28, scherkus wrote: > to be honest I don't think it's worth reusing this particular histogram as > you're: > a) redefining a subset of the enum it uses > b) detecting the type based on URL suffix rather than how > Media.DetectedContainer currently detects the type by inspecting bytes > > considering this is Android specific, I'd by fine having a histogram for > answering the question "Is this an HLS stream or not?" > > > also same question as above w.r.t. doing this inside of > WebMediaPlayerAndroid::load() -- we already have a IsHLSStream() method there :) Done.
https://codereview.chromium.org/505233002/diff/100001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/505233002/diff/100001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:989: void WebMediaPlayerAndroid::InitializePlayer( in the regular HTTP case this may not get called if MediaInfoLoader returns kFailed I'd keep things simple and put this code as early as possible inside load() -- that way we'll ensure consistent counts between the MSE and non-MSE cases
https://codereview.chromium.org/505233002/diff/100001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/505233002/diff/100001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:989: void WebMediaPlayerAndroid::InitializePlayer( I am concerned about url redirections and we may get incorrect count on HLS/non-HLS due to this. We are currently checking HLS on the url passed to load(). This is also problematic due to redirect. Updated the patch to fix the HLS check on the redirected url, and moved the Media.MSE.Playback UMA couting into load(). However, the HLS UMA counting has to stay here due to the redirection issue. On 2014/08/28 21:09:08, scherkus wrote: > in the regular HTTP case this may not get called if MediaInfoLoader returns > kFailed > > I'd keep things simple and put this code as early as possible inside load() -- > that way we'll ensure consistent counts between the MSE and non-MSE cases
https://codereview.chromium.org/505233002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/505233002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:996: if (player_type_ == MEDIA_PLAYER_TYPE_URL) { sanity checking as I want to make sure we get the most accurate data possible ... is checking the GURL::path() suffix enough? would it be better to get the answer from Android's MediaPlayer itself via the metadata extractor? I notice that there's a METADATA_KEY_MIMETYPE key -- would that return the right mimetype?
https://codereview.chromium.org/505233002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/505233002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:996: if (player_type_ == MEDIA_PLAYER_TYPE_URL) { Use media extractor will not work: 1. We don't extract the metadata if the phone is using mobile networks 2. the mimetype may not return correct result. for example, when trying it on ustream.tv, the mime i get is video/wvm rather than application/x-mpegurl So i think sniffing the suffix is probably the best option for now On 2014/08/28 22:15:09, scherkus wrote: > sanity checking as I want to make sure we get the most accurate data possible > ... is checking the GURL::path() suffix enough? > > would it be better to get the answer from Android's MediaPlayer itself via the > metadata extractor? > > I notice that there's a METADATA_KEY_MIMETYPE key -- would that return the right > mimetype?
lgtm
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/505233002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as ac6f87ec2e4601d7072d065e98425cb3c1b8d7a4
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/543663f968c044c1c417187936696e5844c7c4ef Cr-Commit-Position: refs/heads/master@{#292756} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
