1073486 - Temporal patch to disable change resolution in OMX hardware encoder was not being applied

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Oscar Patiño González]

Attachment: define_disable_change_resolution.patch adds the define in the needed file to define MOZ_WEBRTC_OMX

the definition MOZ_WEBRTC_OMX must be defined in video_engine_core.gypi to apply the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1067437.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8495899 [details] [diff] [review]
define_disable_change_resolution.patch adds the define in the needed file to define MOZ_WEBRTC_OMX

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

r+, but lets move it to common.gypi

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

[Blocking Requested - why for this release]:

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48311169d140

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48311169d140

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please nominate this patch for aurora and b2g32 approval when you get a chance :)

Comment 6

[Oscar Patiño González]

Comment on attachment 8495899 [details] [diff] [review]
define_disable_change_resolution.patch adds the define in the needed file to define MOZ_WEBRTC_OMX

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TBPL]:
[Risks and why]: 
[String/UUID change made/needed]:

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495899 [details] [diff] [review]
define_disable_change_resolution.patch adds the define in the needed file to define MOZ_WEBRTC_OMX

I spoke with jesup on irc to get some context for this request. This bug simply sets a build flag to disable resolution switching in a scenario where it will not be functional. This change has already been tested. Aurora+ and b2g32+

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba7f83d8c2b2
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/05c6a4fed6bc

Comment 9

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/05c6a4fed6bc

Comment 10

[William Hsu [:whsu]]

Please provide detailed information (Steps or description) if you need QA to verify this patch.
Thanks.

Comment 11

[Oscar Patiño González]

Please verify it on b2g v2.0 and b2g v2.1

thanks

Comment 12

[William Hsu [:whsu]]

Hi, Oscar,

Could you please provide the reproduce steps and user impact regarding this bug if you need QA to verify it?

Thanks.

Comment 13

[Oscar Patiño González]

We are using phones, the best way to reproduce it is to move very fast  one of the phones, for example from upside to downside continuously. This makes that the image content changes very fast. If you can see video after moving it, then everything is ok. If video 'breaks' (you see a black or green screen and then no video at all) then the test fails.

Thanks

Comment 14

[Oscar Patiño González]

This bug and https://bugzilla.mozilla.org/show_bug.cgi?id=1067437 should be tested together with two phones, since it is a bit more difficult to get fast image changes in a desktop. In fact both bugs are about the same problem, disabling resolution change, but the current bug 1073486 is just a small fix because changes were not being correctly applied.

Thanks!

Comment 15

[William Hsu [:whsu]]

Thanks Oscar! Have a nice day. :)

Add "QAwanted"

Comment 16

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I spent some time on an affected branch (2.1) the day before this issue was reported (in order to have a base for comparison on the latest builds) but I could not reproduce it with the steps on comment 13 after 5 minutes of flipping the phone upside down and back while watching videos.  

Oscar: Is there something wrong with what I was doing?  I cannot verify this issue until I can reproduce it.

Comment 17

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Forgot to give the environmental variables used.

Environmental Variables:
Device: Flame 2.1
BuildID: 20140925071142
Gaia: 86905e14c3ff06a0e6952ba635b6066ad2eea6b4
Gecko: a2e44f9dd87c
Version: 34.0a2 (2.1) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 18

[Oscar Patiño González]

I don't know if you are using flame v184 with new QC firmware. If that's the case you won't see the issue because it is already fixed by firmware.
When I saw the bug, I was using 2.2 and I never tried 2.1, but that shouldn't be a problem, because on 2.1 the "define" was not set either.
I think that you are testing it correctly, the trick is to have fast changes in image so the ratecontrol decides to downgrade the resolution to adjust itself to the fixed bitrate.

Comment 19

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

OK I had been on v180 and could not reproduce so hopefully someone else can try and get this verified for you.