media: Fix FirstApiLevel check in provisioning [chromium/src : main]
Vikram Pasupathy (Gerrit)
Vikram Pasupathy voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang added 2 comments![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.EME.MediaDrm.FirstApiLevel", first_api_level);This should be integers with a defined range? Maybe it's not "sparse"?
// released before "ro.product.first_api_level" was introduced.In this case, is it possible that the SKD version is less than O? I don't know when the first_api_level was introduced...
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 2 comments![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.EME.MediaDrm.FirstApiLevel", first_api_level);This should be integers with a defined range? Maybe it's not "sparse"?
AW team does the same here for their ApiLevel:
So I just copied that, which I think makes sense.
// released before "ro.product.first_api_level" was introduced.In this case, is it possible that the SKD version is less than O? I don't know when the first_api_level was introduced...
I also took a look at that, this document http://shortn/_gsiTlfzxII says it was introduced in between MR and N.
I sanity checked it by googling "ro.product.first_api_level" Nougat and saw some dumped builds where this was in fact set to Nougats value, which is pre O.
I looked at the lifetime support for a couple of devices, and it looks like most of them ended support at Pie, which is actually being deprecated currently, but the Pixel is supported til Quince, which Chromium actively supports.
So thats why this UMA will be useful to see how many of these old clients still use this flow, and if we can remove this or not.
Related details
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Xiaohan Wang
| Code-Review | +1 |
3 comments
base::UmaHistogramSparse("Media.EME.MediaDrm.FirstApiLevel", first_api_level);Vikram PasupathyThis should be integers with a defined range? Maybe it's not "sparse"?
AW team does the same here for their ApiLevel:
So I just copied that, which I think makes sense.
Thanks for the example. I don't know what's the recommendataion is here.
Please keep this unresolved so the metrics reviewer can take a look.
} else if (first_api_level == 0) {nit: No need to use else as we return early.
```
if (first_api_level >= base::android::android_info::SDK_VERSION_OREO) {
return true;
}
if (first_api_level == 0) {
return ...
}return false;
```
// released before "ro.product.first_api_level" was introduced.Vikram PasupathyIn this case, is it possible that the SKD version is less than O? I don't know when the first_api_level was introduced...
I also took a look at that, this document http://shortn/_gsiTlfzxII says it was introduced in between MR and N.
I sanity checked it by googling "ro.product.first_api_level" Nougat and saw some dumped builds where this was in fact set to Nougats value, which is pre O.
I looked at the lifetime support for a couple of devices, and it looks like most of them ended support at Pie, which is actually being deprecated currently, but the Pixel is supported til Quince, which Chromium actively supports.
So thats why this UMA will be useful to see how many of these old clients still use this flow, and if we can remove this or not.
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
nit: No need to use else as we return early.
```
if (first_api_level >= base::android::android_info::SDK_VERSION_OREO) {
return true;
}if (first_api_level == 0) {
return ...
}return false;
```
- Evan Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Frias (Gerrit)
Rahul Frias added 1 comment![]( "Open in Gerrit")
// released before "ro.product.first_api_level" was introduced.Vikram PasupathyIn this case, is it possible that the SKD version is less than O? I don't know when the first_api_level was introduced...
Xiaohan WangI also took a look at that, this document http://shortn/_gsiTlfzxII says it was introduced in between MR and N.
I sanity checked it by googling "ro.product.first_api_level" Nougat and saw some dumped builds where this was in fact set to Nougats value, which is pre O.
I looked at the lifetime support for a couple of devices, and it looks like most of them ended support at Pie, which is actually being deprecated currently, but the Pixel is supported til Quince, which Chromium actively supports.
So thats why this UMA will be useful to see how many of these old clients still use this flow, and if we can remove this or not.
Looks good to me. Please double check with Android owners.
- Evan Liu
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Xiaohan Wang
| Code-Review | +1 |
2 comments
// released before "ro.product.first_api_level" was introduced.Vikram PasupathyIn this case, is it possible that the SKD version is less than O? I don't know when the first_api_level was introduced...
Xiaohan WangI also took a look at that, this document http://shortn/_gsiTlfzxII says it was introduced in between MR and N.
I sanity checked it by googling "ro.product.first_api_level" Nougat and saw some dumped builds where this was in fact set to Nougats value, which is pre O.
I looked at the lifetime support for a couple of devices, and it looks like most of them ended support at Pie, which is actually being deprecated currently, but the Pixel is supported til Quince, which Chromium actively supports.
So thats why this UMA will be useful to see how many of these old clients still use this flow, and if we can remove this or not.
Rahul FriasLooks good to me. Please double check with Android owners.
lgtm
Acknowledged
base::android::android_info::SDK_VERSION_OREO;Since OREO is deprecated, will this always return true for now?
Related details
- Evan Liu
- Rahul Frias
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
base::android::android_info::SDK_VERSION_OREO;Since OREO is deprecated, will this always return true for now?
Yeah I think that makes sense, the only reason I wasn't confident to do it now was because what if there is a device running MR_1 that somehow upgraded to Q? We can keep this for now, and then based on the metric, there are two options:
1. If there are no zeroes, then we can remove the sdkint() > Oreo.
2. If there are no first level versions less than Oreo in general, then we can deprecate the whole first level check, and remove the code.
Thoughts?
Related details
- Evan Liu
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
base::android::android_info::SDK_VERSION_OREO;Vikram PasupathySince OREO is deprecated, will this always return true for now?
Yeah I think that makes sense, the only reason I wasn't confident to do it now was because what if there is a device running MR_1 that somehow upgraded to Q? We can keep this for now, and then based on the metric, there are two options:
1. If there are no zeroes, then we can remove the sdkint() > Oreo.
2. If there are no first level versions less than Oreo in general, then we can deprecate the whole first level check, and remove the code.
Thoughts?
- Evan Liu
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron voted and added 3 comments![]( "Open in Gerrit")
Votes added by Johannes Kron
| Code-Review | +1 |
3 comments
base::UmaHistogramSparse("Media.EME.MediaDrm.FirstApiLevel", first_api_level);Vikram PasupathyThis should be integers with a defined range? Maybe it's not "sparse"?
Xiaohan WangAW team does the same here for their ApiLevel:
So I just copied that, which I think makes sense.
Thanks for the example. I don't know what's the recommendataion is here.
Please keep this unresolved so the metrics reviewer can take a look.
Thanks for checking.
Confirming that UmaHistogramSparse is correct here. Sparse handles the potential gaps in API levels well and ensures we don't need to constantly update a 'max' value like we would with UmaHistogramExactLinear.
<histogram name="Media.EME.MediaDrm.FirstApiLevel" enum="AndroidApiLevel"I'm not sure of this, but since this histogram is in media/, you might need to move AndroidApiLevel from android/enums.xml to the top-level tools/metrics/histograms/enums.xml so that it is accessible here.
Related details
- Evan Liu
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy removed Evan Liu from this change![]( "Open in Gerrit")
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 2 comments![]( "Open in Gerrit")
PTAL at it again, I moved the AndroidApiLevel to the top level enums.xml from the android/enums.xml
<histogram name="Media.EME.MediaDrm.FirstApiLevel" enum="AndroidApiLevel"I'm not sure of this, but since this histogram is in media/, you might need to move AndroidApiLevel from android/enums.xml to the top-level tools/metrics/histograms/enums.xml so that it is accessible here.
- Johannes Kron
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron voted Code-Review+1![]( "Open in Gerrit")
- Vikram Pasupathy
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From analysis/uma/chrome-metrics.gwsq:
Histograms should by default be reviewed by the owners of the subdirectories. The chromium-met...@google.com gwsq should be used when there are no individual owners, or for escalation to the Metrics team.
If you are interested in becoming a metrics reviewer, please review the instructions at https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Becoming-a-Metrics-Reviewer
Reviewer source(s):
rka...@chromium.org is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- Robert Kaplow
- Vikram Pasupathy
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
Bump for Xiaohan and Robert, PTAL
(lgtms were lost).
Related details
- Robert Kaplow
- Xiaohan Wang
Xiaohan Wang (Gerrit)
Xiaohan Wang voted Code-Review+1![]( "Open in Gerrit")
- Robert Kaplow
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Kaplow (Gerrit)
Robert Kaplow voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
media: Fix FirstApiLevel check in provisioning
The FirstApiLevel check was introduced because devices that were
introduced with Android N and previous versions had immutable behavior
changes linked to the devices at the time of release that needed to be
checked.
However, there exists a bug currently, where if a device was released
during Android N, and it currently runs a version of Android like Q,
this could would state a faulty response that per application
provisioning is supported on this device, when in fact, it does not.
This now fixes this behavior by only comparing against the current
android SDK int to Oreo IF and only if the first_api_level is unset,
which is indicated by a value of 0, which is mentioned in
http://shortn/_tonWP0thCA, the design document.
Note that "ro.product.first_api_level" was introduced during Android L
so we shouldn't see many values of unset on current chrome milestones.
To gain clarity over what FirstApiLevels we are seeing during the
provisioning in MediaDrm, we also introduce a histogram to be recorded
on this check.
Tested by verifying the histogram reports the first Level. The phone I
tested on is currently running Android 16 (API Level 36), but the
histogram reports 34, which makes sense because the phone is a Pixel 8a
released in 2024 May.
- M media/base/android/media_drm_bridge.cc
- M tools/metrics/histograms/enums.xml
- M tools/metrics/histograms/metadata/android/enums.xml
- M tools/metrics/histograms/metadata/media/histograms.xml
Code-Review: +1 by Johannes Kron, +1 by Robert Kaplow, +1 by Xiaohan Wang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |