1206296 - Remove last bits of a Froyo-specific media code in libcubeb and OMX plugin

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect)

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: 1041882.-Remove-Froyo-specific-support-from-libc.patch

Just uploading before I hop on a cab to the airport.

Comment 2

[Mike Taylor [:miketaylr]]

Attachment: 1041882.-Remove-Froyo-specific-OMX-plugin-suppor.patch

Comment 3

[Mike Taylor [:miketaylr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a06b17fbdfbf

Comment 4

[Mike Taylor [:miketaylr]]

Comment on attachment 8663185 [details] [diff] [review]
1041882.-Remove-Froyo-specific-support-from-libc.patch

Jean-Yves, would you mind reviewing?

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8663185 [details] [diff] [review]
1041882.-Remove-Froyo-specific-support-from-libc.patch

Review of attachment 8663185 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but I don't think I'm the best person to review it. I have no idea of what that was for (can only guess looking at the code) ; nor the rationale behind removing support for 2.2 .. snorp is probably the person that should look into it.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8663186 [details] [diff] [review]
1041882.-Remove-Froyo-specific-OMX-plugin-suppor.patch

Review of attachment 8663186 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, passing on to snorp

Comment 7

[Mike Taylor [:miketaylr]]

Thanks for taking a look Jean-Yves!

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8663185 [details] [diff] [review]
1041882.-Remove-Froyo-specific-support-from-libc.patch

Review of attachment 8663185 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, but maybe padenot should look?

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8663186 [details] [diff] [review]
1041882.-Remove-Froyo-specific-OMX-plugin-suppor.patch

Review of attachment 8663186 [details] [diff] [review]:
-----------------------------------------------------------------

We have basically no testing for this. Did you confirm that media still works on a 2.3 device?

::: media/omx-plugin/OmxPlugin.cpp
@@ +30,5 @@
>  
>  #undef LOG
>  #define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "OmxPlugin" , ## args)
>  
> +#if !defined(MOZ_ANDROID_GB) && !defined(MOZ_ANDROID_HC)

I think you broke GB. We need to define MOZ_ANDROID_V2_X_X there, according to previous logic.

@@ +267,5 @@
>    // 16 = Force hardware decoding
>    int32_t flags = 0;
>    aPluginHost->GetIntPref("media.stagefright.omxcodec.flags", &flags);
>    if (flags != 0) {
> +#if !defined(MOZ_ANDROID_GB)

Ah, nevermind.

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8663185 [details] [diff] [review]
1041882.-Remove-Froyo-specific-support-from-libc.patch

Review of attachment 8663185 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah looks good.

Comment 11

[Mike Taylor [:miketaylr]]

> We have basically no testing for this. Did you confirm that media still works on a 2.3 device?

Uhhh, not yet. But I'll do that before landing.

Thanks for the reviews padenot and snorp.

Comment 12

[Mike Taylor [:miketaylr]]

Attachment: 1041882-2.-Remove-Froyo-specific-support-from-libc.patch

Updating commit message to reflect r+, carrying forward r+.

Comment 13

[Mike Taylor [:miketaylr]]

Attachment: 1041882-2.-Remove-Froyo-specific-OMX-plugin-suppor.patch

Ditto.

Still haven't managed to get my hands on a Gingerbread device to test this, but I'll be in the Paris office next week -- maybe someone will have one to lend me.

Comment 14

[Jean-Yves Avenard [:jya]]

I have an HTC with android 2.2. Will that help?

Comment 16

[Mike Taylor [:miketaylr]]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> I have an HTC with android 2.2. Will that help?

I've got an old Desire on 2.2 as well. ^_^ 
I just need to manually verify that nothing breaks on 2.3 before feeling confident enough to land this.

Comment 17

[Mike Taylor [:miketaylr]]

OK, padenot lent me a 2.3 device he found in a pile of dusty phones and these patches play the same videos and audio as a regular Nightly (mp4 is kinda glitchy, but I assume that's because it's running on an underpowered device or something).

I tested with http://techslides.com/sample-webm-ogg-and-mp4-video-files-for-html5 and http://hpr.dogphilosophy.net/test/ for audio.

One thing to note is the webm and ogg videos don't stretch to the full size of the player and are surrounded by green (or pink), but that happens in Nightly with or without this patchset. ¯\_(ツ)_/¯

Comment 18

[Jean-Yves Avenard [:jya]]

(In reply to Mike Taylor [:miketaylr] from comment #17)
> One thing to note is the webm and ogg videos don't stretch to the full size
> of the player and are surrounded by green (or pink), but that happens in
> Nightly with or without this patchset. ¯\_(ツ)_/¯

Was that with bug 1190379 or without ?

the libvpx decoder properly handles margins , but I doubt anyone tried to check how the android's built-in decoder manages those.

Comment 19

[Jean-Yves Avenard [:jya]]

ah never mind, that's 2.3, AndroidDecoderModule is only available on 4.2 and later.

Comment 20

[Mike Taylor [:miketaylr]]

Fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1041882#c5.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8d280b9d4fe
https://hg.mozilla.org/mozilla-central/rev/0b4987e8fe7d