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

Status: RESOLVED FIXED

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

Description

[mzanaty]

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.

Comment 1

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

Attachment: 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

Comment 2

[Byron Campen [:bwc]]

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.

Comment 3

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

(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

[Byron Campen [:bwc]]

We also have a TODO comment to a closed bug here.

Comment 5

[mzanaty]

(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).

Comment 6

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

(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)

Comment 7

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

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

Comment 8

[Byron Campen [:bwc]]

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.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a202e2f54b45

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2b60f30db362 for something in that push causing b2g build bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=47078117&tree=Mozilla-Inbound

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/899bffa59af5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/899bffa59af5

Comment 13

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

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

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c42c04e31a7