WebRTC H.264 HD video needs higher level, resolution, frame rate, bit rate

RESOLVED FIXED in Firefox 33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mzanaty, Assigned: jesup)

Tracking

(Blocks: 1 bug)

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
WebRTC H.264 video is currently limited to a low level (1.2). Apps should be able to request HD video (or any arbitrary resolution, frame rate and bit rate), and the underlying engine should generate higher levels when creating OpenH264 encoder and decoder instances so they reserve the appropriate memory resources needed for the max level. For example, level 3.1 for 1280x720p 30fps, level 4.0 for 1920x1080p 30 fps, etc. Note that other (often dynamic) limits, such as CPU or network bandwidth, may prevent the engine from reaching the max level limits even after creating codec instances using the max level.
(Assignee)

Comment 1

4 years ago
Created attachment 8481087 [details] [diff] [review]
Make H.264 Level configurable and change OpenH264 default to 3.1

bumped to 3.1, and made it configurable.  Tradeoff of memory use vs capability and possible available bandwidth; note we cap at 2Mbps now
(Assignee)

Updated

4 years ago
Attachment #8481087 - Flags: review?(docfaraday)

Comment 2

4 years ago
Comment on attachment 8481087 [details] [diff] [review]
Make H.264 Level configurable and change OpenH264 default to 3.1

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

A question and a comment.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2294,5 @@
>    // XXX See bug 1043515 - we may want to support a higher profile than
>    // 1.3, depending on hardware(?)
> +  // For OMX, constrained baseline level 1.2
> +  // Max resolution CIF; we should include max-mbps
> +  int32_t level = 13;

Should this be 12? Or does the comment need to change?

@@ +2300,5 @@
> +  vcmGetVideoLevel(0, &level);
> +  level &= 0xFF;
> +  level |= 0x42E000;
> +
> +  return (uint32_t) level;

I guess I would have a separate uint32_t |result| that I roll the level into, since that's what you'll need if we ever make other parts of this configurable, but that's not a big difference.
(Assignee)

Comment 3

4 years ago
(In reply to Byron Campen [:bwc] from comment #2)
> Comment on attachment 8481087 [details] [diff] [review]
> Make H.264 Level configurable and change OpenH264 default to 3.1
> 
> Review of attachment 8481087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A question and a comment.
> 
> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> @@ +2294,5 @@
> >    // XXX See bug 1043515 - we may want to support a higher profile than
> >    // 1.3, depending on hardware(?)
> > +  // For OMX, constrained baseline level 1.2
> > +  // Max resolution CIF; we should include max-mbps
> > +  int32_t level = 13;
> 
> Should this be 12? Or does the comment need to change?

I added "via a pref" - it's overridden in the pref on hardware that wants something different.  1.3 is the suggested minimum for WebRTC.  I've used the pref to set 3.1 (1280x720@30p) for OpenH264

This is really just a backstop anyways

Comment 4

4 years ago
We also have a TODO comment to a closed bug here.
(Reporter)

Comment 5

4 years ago
(In reply to Byron Campen [:bwc] from comment #2)
> > +  vcmGetVideoLevel(0, &level);
> > +  level &= 0xFF;
> > +  level |= 0x42E000;
> > +
> > +  return (uint32_t) level;
> 
> I guess I would have a separate uint32_t |result| that I roll the level
> into, since that's what you'll need if we ever make other parts of this
> configurable, but that's not a big difference.

We do plan to add other profiles to OpenH264, starting with (Constrained) High Profile. And many OMX HW codecs already support multiple profiles (including HP). I can open a separate bug on supporting configurable profiles, since that would be a longer term item (esp. for OpenH264 which currently lacks other profiles).
(Assignee)

Comment 6

4 years ago
(In reply to mzanaty from comment #5)
> (In reply to Byron Campen [:bwc] from comment #2)
> > > +  vcmGetVideoLevel(0, &level);
> > > +  level &= 0xFF;
> > > +  level |= 0x42E000;
> > > +
> > > +  return (uint32_t) level;
> > 
> > I guess I would have a separate uint32_t |result| that I roll the level
> > into, since that's what you'll need if we ever make other parts of this
> > configurable, but that's not a big difference.
> 
> We do plan to add other profiles to OpenH264, starting with (Constrained)
> High Profile. And many OMX HW codecs already support multiple profiles
> (including HP). I can open a separate bug on supporting configurable
> profiles, since that would be a longer term item (esp. for OpenH264 which
> currently lacks other profiles).

And those should be retrieved *from* the plugin or plugin meta-information, not configured into Mozilla, anyways.  (arguably the level should too, but we're not doing that for 33/34)
(Assignee)

Comment 7

4 years ago
I can pull the old comment; I left it somewhat on purpose, but don't really care, and others may be confused

Comment 8

4 years ago
Comment on attachment 8481087 [details] [diff] [review]
Make H.264 Level configurable and change OpenH264 default to 3.1

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

Ok with some comment changes.
Attachment #8481087 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a202e2f54b45
Whiteboard: [openh264-uplift]
Target Milestone: --- → mozilla34
Blocks: 948160
https://hg.mozilla.org/mozilla-central/rev/899bffa59af5
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
status-firefox33: --- → affected
status-firefox34: --- → fixed
(Assignee)

Comment 13

4 years ago
Comment on attachment 8481087 [details] [diff] [review]
Make H.264 Level configurable and change OpenH264 default to 3.1

Approval Request Comment
[Feature/regressing bug #]: openh264

[User impact if declined]: Limits H264 sending and reception to level 1.3, which is only CIF/QVGA@30 (QVGA = 320x240).  Update makes it configurable via a pref, and sets the default for OpenH264 to level 3.1 (1280x720@30

[Describe test coverage new/current, TBPL]: Regular TBPL testing will use the level in negotiation and with the fake H.264 plugin.  Real plugin use requires manual testing; the plugin is known to work in testing up to level 4 (1920x1080@30)

[Risks and why]: virtually none - moves hard-coded value to a pref

[String/UUID change made/needed]: none
Attachment #8481087 - Flags: approval-mozilla-aurora?
Attachment #8481087 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 14

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/8c42c04e31a7
status-firefox33: affected → fixed
Whiteboard: [openh264-uplift]
You need to log in before you can comment on or make changes to this bug.