1024304 - External encoder's IDR Request can't be called in WebRTC Gecko Media Plugins adapter

Status: RESOLVED DUPLICATE of bug 1020760

(Core :: WebRTC, defect)

Description

[karina]

User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/5.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; InfoPath.3; .NET4.0C; .NET4.0E)

Steps to reproduce:

1. enable h264 codec
2. test on https://apprtc.appspot.com 
3. add breakpoint on VideoSender::IntraFrameRequest() (video_sender.cc  398 line in WebRTC)  
4. check whether _encoder->RequestFrame() is called.


Actual results:

_encoder->RequestFrame() isn't called. (_encoder->InternalSource() is false)




Expected results:

_encoder->RequestFrame is called. Then h264 encoder can encode a frame as IDR frame.

Comment 1

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

(In reply to karina from comment #0)

> 1. enable h264 codec
> 2. test on https://apprtc.appspot.com 
> 3. add breakpoint on VideoSender::IntraFrameRequest() (video_sender.cc  398
> line in WebRTC)  
> 4. check whether _encoder->RequestFrame() is called.
> 
> Actual results:
> 
> _encoder->RequestFrame() isn't called. (_encoder->InternalSource() is false)
> 
> Expected results:
> 
> _encoder->RequestFrame is called. Then h264 encoder can encode a frame as
> IDR frame.

I'm afraid that's not how it's supposed to work.

IntraFrameRequest() is called, and for an external codec (i.e. h.264), it sets the _nextFrameTypes[0] to kVideoFrameKey.  When it next encodes a frame, it calls AddVideoFrame (right above), and kVideoFrameKey gets translates to kKeyFrame, which is translated to kGMPKeyFrame, and passed to the GMP Encode() call.

This should all work today. It does trigger (on Linux) a shared-memory-limit bug when we stop decoding due to a frame drop (I'm working on this separately).  Note also that in most connections, the error recoveries will (mostly) be via NACK and not IDR, unless you explicitly disable NACK or on long-RTT connections.

I tried instead forcing IntraFrames every 30 frames or so.  It didn't work, so debugging through it there appears to be a bug in the OpenH264 codec impl inside the plugin somewhere.

Encoded frame, size = 1274, first NAL type = 0x61
Forcing key frame: 0x0x7f17b2eaf800
Encoding GMP keyframe
Encode: frameTypes[0] = 0
Child: frame type 0
Encoded frame, size = 1083, first NAL type = 0x61
Encode: frameTypes[0] = 1
Child: frame type 1
Encoded frame, size = 1172, first NAL type = 0x61

The "Child" references are the aFrameType array values in the child, and as you can see when we request an iframe it gets to the child (in RecvEncode()).  And we don't get an iframe in response.

I'll upload my debugging patch here.

My apologies for the delay digging in; I'd thought this was just a "it's not hooked up" issue.

Comment 2

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

Attachment: debugs for IDRs NOT FOR CHECKIN

Comment 3

[karina]

where do you get openh264 codec from? Cisco/master branch had supported "forceIntraframe"

Comment 4

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

plugin4-merge-gmpdir, which may well be behind the times (that's the version I was told to use by ehugg a while ago).   If you see IDRs in response to requests with my patch applied, then everything should be good.  Could you try that?

Comment 5

[Ethan Hugg [:ehugg]]

The plugin build was merged into github.com/cisco/openh264 master on the 17th after your review.  It should now work on the master branch if you build the plugin with 'make plugin'.

Comment 6

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

With an updated plugin from "master" branch:
https://pastebin.mozilla.org/5478686

IDRs are generated, but the frame sizes seem VERY non-CBR (and odd).
Also, with that build, the display was pretty badly corrupted all the time, and with IDRs it shoulnd't be.

Comment 7

[karina]

thanks for your feedback. I will try it to check the quality.

Comment 8

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

I found a 1-liner change that seems to fix the video - we're using Mode 1 encoding, but were telling the encoder the max packet size was ~1200 bytes (effectively it was generating mode 0, which should work, but obviously there's a problem (and the webrtc.org code may have troubles with that.

Changing "GMPVideoErr err = mGMP->InitEncode(codec, this, 1, aMaxPayloadSize);" to
  GMPVideoErr err = mGMP->InitEncode(codec, this, 1, 64000/*aMaxPayloadSize*/);  (line 150 of WebrtcGmpVideoCodec.cpp) effectively makes it generate mode 1 NALs, and the video is now clean

Comment 9

[Ethan Hugg [:ehugg]]

The Video looks good on the plugin I built on the 20th, but building today's has this badly corrupted display.  The the older versions of the plugin seemed to handle this.

Comment 10

[Ethan Hugg [:ehugg]]

Attachment: Hardset maxpayload size for GMP InitEncode - Not For Checkin

Comment 11

[Ethan Hugg [:ehugg]]

Uploaded Jesup's hardcoding of MaxPayloadSize on InitEncode for folks that need to get the new version of the OpenH264 plugin to work.  

I would like the plugin to work as it did a week ago and be resilient to this so that it works with the FF32 train again.

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 8447312 [details] [diff] [review]
Hardset maxpayload size for GMP InitEncode - Not For Checkin


OpenH264 plugin should work again without this workaround.

Comment 13

[karina]

Openh264 plugin work well based on disabled maxpayload size setting. when turn on this setting,the video will display green block.
setting maxpayload means that openh264 will turn on multi-slice mode.
it means that there is no a good support for multi-slice mode with FF32. But it isn't belong to this issue.I will submit a new bug.

Comment 14

[karina]

Openh264 plugin work well based on disabled maxpayload size setting. when turn on this setting,the video will display green block.
setting maxpayload means that openh264 will turn on multi-slice mode.
it means that there is no a good support for multi-slice mode with FF32. But it isn't belong to this issue.I will submit a new bug.

Comment 15

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

1020760 handles setting the packet size larger (and with bug 1022012, the plugin can be smarter about reading the mode-1 setting and if so ignoring the packet size)