Closed Bug 1073486 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
blocking-b2g 2.0+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: opatinobugzilla, Assigned: opatinobugzilla)

References

Details

(Keywords: verifyme)

Attachments

(1 file)

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.
Attachment #8495899 - Flags: review?(rjesup)
Assignee: nobody → opatinobugzilla
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
Attachment #8495899 - Flags: review?(rjesup) → review+
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Whiteboard: [webrtc-uplift]
https://hg.mozilla.org/mozilla-central/rev/48311169d140
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: verifyme
blocking-b2g: 2.0? → 2.0+
Please nominate this patch for aurora and b2g32 approval when you get a chance :)
Flags: needinfo?(opatinobugzilla)
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:
Attachment #8495899 - Flags: approval-mozilla-b2g32?
Attachment #8495899 - Flags: approval-mozilla-aurora?
Flags: needinfo?(opatinobugzilla)
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+
Attachment #8495899 - Flags: approval-mozilla-b2g32?
Attachment #8495899 - Flags: approval-mozilla-b2g32+
Attachment #8495899 - Flags: approval-mozilla-aurora?
Attachment #8495899 - Flags: approval-mozilla-aurora+
Whiteboard: [webrtc-uplift]
Please provide detailed information (Steps or description) if you need QA to verify this patch.
Thanks.
Keywords: verifyme
Flags: needinfo?(opatinobugzilla)
Please verify it on b2g v2.0 and b2g v2.1

thanks
Flags: needinfo?(opatinobugzilla)
Hi, Oscar,

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

Thanks.
Flags: needinfo?(opatinobugzilla)
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
Flags: needinfo?(opatinobugzilla)
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!
Thanks Oscar! Have a nice day. :)

Add "QAwanted"
Keywords: qawanted
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.
Flags: needinfo?(opatinobugzilla)
Flags: needinfo?(jmitchell)
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
Flags: needinfo?(jmitchell)
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.
Flags: needinfo?(opatinobugzilla)
OK I had been on v180 and could not reproduce so hopefully someone else can try and get this verified for you.
QA Whiteboard: [QAnalyst-Triage+]
Keywords: qawantedverifyme
You need to log in before you can comment on or make changes to this bug.