Force preload=none and disable autoplay on lowend Android devices.

Force preload=none and disable autoplay on lowend Android devices.

Some older devices are unable to maintain multiple concurrent
players without borking the system badly enough that it needs
a reboot in order to continue playing media.

We also disable autoplay and preloading on low-memory devices to
avoid unnecessary codec churn.

BUG=612909, 622826
TEST=less crashy JB devices.

Committed: https://crrev.com/cd97125235326c121186b01e0b3484a60a98e791
Cr-Commit-Position: refs/heads/master@{#420073}

[DaleCurtis, 2016-09-16 22:39:27]

dalecurtis@chromium.org changed reviewers:
+ foolip@chromium.org, mlamouri@chromium.org

[DaleCurtis, 2016-09-16 22:39:28]

Probably we should disable autoplay on these devices as well.

[DaleCurtis, 2016-09-16 23:02:09]

Actually, for sure we should disable autoplay with this too, see
http://crbug.com/622826

[DaleCurtis, 2016-09-16 23:31:02]

Description was changed from

==========
Force preload=none on older JellyBean devices.

Some older devices are unable to maintain multiple concurrent
players without borking the system badly enough that it needs
a reboot in order to continue playing media.

BUG=612909
TEST=less crashy JB devices.
==========

to

==========
Force preload=none and disable autoplay on lowend Android devices.

Some older devices are unable to maintain multiple concurrent
players without borking the system badly enough that it needs
a reboot in order to continue playing media.

We also disable autoplay and preloading on low-memory devices to
avoid unnecessary codec churn.

BUG=612909
TEST=less crashy JB devices.
==========

[DaleCurtis, 2016-09-16 23:31:17]

Description was changed from

==========
Force preload=none and disable autoplay on lowend Android devices.

Some older devices are unable to maintain multiple concurrent
players without borking the system badly enough that it needs
a reboot in order to continue playing media.

We also disable autoplay and preloading on low-memory devices to
avoid unnecessary codec churn.

BUG=612909
TEST=less crashy JB devices.
==========

to

==========
Force preload=none and disable autoplay on lowend Android devices.

Some older devices are unable to maintain multiple concurrent
players without borking the system badly enough that it needs
a reboot in order to continue playing media.

We also disable autoplay and preloading on low-memory devices to
avoid unnecessary codec churn.

BUG=612909, 622826
TEST=less crashy JB devices.
==========

[Srirama, 2016-09-17 08:45:03]

srirama.m@samsung.com changed reviewers:
+ srirama.m@samsung.com

[Srirama, 2016-09-17 08:45:04]

https://codereview.chromium.org/2344353003/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2344353003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:1152:
base::android::BuildInfo::GetInstance()->sdk_int() <= 18 ||
how about using "SDK_VERSION_JELLY_BEAN_MR2" instead of 18?

[foolip, 2016-09-19 13:58:55]

A test would be good, see
LayoutTests/media/video-force-preload-none-to-metadata-on-load.html

https://codereview.chromium.org/2344353003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2344353003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2006: ||
(document().settings() && (document().settings()->dataSaverEnabled() ||
document().settings()->forcePreloadNoneForMediaElements()))) {
Makes sense, as long as it's OK that calling play() or seeking will start the
load.

[foolip, 2016-09-19 13:59:55]

rs lgtm if I'm not responsive when the test is added and someone else reviews
it.

[DaleCurtis, 2016-09-19 21:04:45]

dalecurtis@chromium.org changed reviewers:
+ esprehn@chromium.org

[DaleCurtis, 2016-09-19 21:04:46]

Test added. +esprehn for content/renderer review since it touches on Blink
WebSettings API.

https://codereview.chromium.org/2344353003/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2344353003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:1152:
base::android::BuildInfo::GetInstance()->sdk_int() <= 18 ||
On 2016/09/17 at 08:45:04, Srirama wrote:
> how about using "SDK_VERSION_JELLY_BEAN_MR2" instead of 18?

Done.

https://codereview.chromium.org/2344353003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2344353003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2006: ||
(document().settings() && (document().settings()->dataSaverEnabled() ||
document().settings()->forcePreloadNoneForMediaElements()))) {
On 2016/09/19 at 13:58:54, foolip wrote:
> Makes sense, as long as it's OK that calling play() or seeking will start the
load.

Yes, this is fine since it's still relatively protected by a user gesture. When
we enabled Spitzer we brought in some automatic preload which is too much for
these devices, so this restores us to the pre-Spitzer state where the user has
to shoot themselves in their own foot instead of us doing it for them!

Still not great, but there's no guarantee with these older devices (even force
pausing and releasing the resources of the media is insufficient when the device
is in a near oom state).

[DaleCurtis, 2016-09-20 20:21:54]

esprehn, mlamouri: Ping?

[esprehn, 2016-09-20 22:04:56]

lgtm

[DaleCurtis, 2016-09-20 23:11:39]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 23:12:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 23:33:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 23:33:43]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[DaleCurtis, 2016-09-20 23:47:16]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 23:47:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 00:56:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 00:56:14]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2016-09-21 16:00:58]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-09-21 16:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2344353003/#ps60001
(title: "Fix enum namespace.")

[commit-bot: I haz the power, 2016-09-21 16:01:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 16:09:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-21 16:12:53]

Description was changed from

==========
Force preload=none and disable autoplay on lowend Android devices.

Some older devices are unable to maintain multiple concurrent
players without borking the system badly enough that it needs
a reboot in order to continue playing media.

We also disable autoplay and preloading on low-memory devices to
avoid unnecessary codec churn.

BUG=612909, 622826
TEST=less crashy JB devices.
==========

to

==========
Force preload=none and disable autoplay on lowend Android devices.

Some older devices are unable to maintain multiple concurrent
players without borking the system badly enough that it needs
a reboot in order to continue playing media.

We also disable autoplay and preloading on low-memory devices to
avoid unnecessary codec churn.

BUG=612909, 622826
TEST=less crashy JB devices.

Committed: https://crrev.com/cd97125235326c121186b01e0b3484a60a98e791
Cr-Commit-Position: refs/heads/master@{#420073}
==========

[commit-bot: I haz the power, 2016-09-21 16:12:55]

Patchset 4 (id:??) landed as
https://crrev.com/cd97125235326c121186b01e0b3484a60a98e791
Cr-Commit-Position: refs/heads/master@{#420073}

[mlamouri (slow - plz ping), 2016-09-22 07:49:32]

Sorry the delay, I was off on Friday and at an offsite/conference this week.

I would have preferred if the change was limited to JB. It's unclear why it's
including low end devices apart for precautions. Do we have data regarding
issues specific to low end devices that are not JB? My concern is that as much
as JB devices are old and not common (3-5%) of Chrome Android usage, low end
devices are still being sold and might be more common (I have no data).

Also, you don't test the change for low end devices and you are introducing a
Java call from the renderer process which is usually frowned upon.

https://codereview.chromium.org/2344353003/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2344353003/diff/60001/content/renderer/render...
content/renderer/render_view_impl.cc:1154: base::SysInfo::IsLowEndDevice();
This ends up doing a Java call from the renderer/ which we usually don't like.

[DaleCurtis, 2016-09-22 17:02:31]

I simulated testing of low end behavior on an A1 device; which while not 512mb,
is already a pretty bad experience (slow processor, single process, etc) -- this
was an improvement on that device. If you have an actual 512mb device, please
give it a go and let me know. I'm happy to revert the SysInfo portions if you
feel strongly.

I wasn't aware that SysInfo is frowned upon in the renderer, this is the case
that should be documented in the function signature. Do you have a link to
details on why this is frowned upon? In any case, the value is cached and always
pulled during RenderProcess startup:

https://cs.chromium.org/chromium/src/base/sys_info_android.cc?l=218
https://cs.chromium.org/chromium/src/content/renderer/render_process_impl.cc?...