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

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: miketaylr, Assigned: miketaylr)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
Assignee: nobody → miket
Blocks: 1041882
Created attachment 8663185 [details] [diff] [review]
1041882.-Remove-Froyo-specific-support-from-libc.patch

Just uploading before I hop on a cab to the airport.
Created attachment 8663186 [details] [diff] [review]
1041882.-Remove-Froyo-specific-OMX-plugin-suppor.patch
Comment on attachment 8663185 [details] [diff] [review]
1041882.-Remove-Froyo-specific-support-from-libc.patch

Jean-Yves, would you mind reviewing?
Attachment #8663185 - Flags: review?(jyavenard)
Attachment #8663186 - Flags: review?(jyavenard)
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.
Attachment #8663185 - Flags: review?(jyavenard) → review?(snorp)
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
Attachment #8663186 - Flags: review?(jyavenard) → review?(snorp)
Thanks for taking a look Jean-Yves!
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?
Attachment #8663185 - Flags: review?(snorp)
Attachment #8663185 - Flags: review?(padenot)
Attachment #8663185 - Flags: review+
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.
Attachment #8663186 - Flags: review?(snorp) → review+
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.
Attachment #8663185 - Flags: review?(padenot) → review+
> 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.
Created attachment 8666370 [details] [diff] [review]
1041882-2.-Remove-Froyo-specific-support-from-libc.patch

Updating commit message to reflect r+, carrying forward r+.
Attachment #8663185 - Attachment is obsolete: true
Attachment #8666370 - Flags: review+
Created attachment 8666371 [details] [diff] [review]
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.
Attachment #8663186 - Attachment is obsolete: true
Attachment #8666371 - Flags: review+
I have an HTC with android 2.2. Will that help?
Duplicate of this bug: 1054336
(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.
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. ¯\_(ツ)_/¯
Keywords: checkin-needed
(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.
ah never mind, that's 2.3, AndroidDecoderModule is only available on 4.2 and later.
Fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1041882#c5.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.