1002402 - Support RTP H.264 input data in WebRTC OMX decoder.

Status: RESOLVED FIXED

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

Description

[John Lin [:jhlin][:jolin]]

Per bug 911046 comment 134, H.264 input data from RTP doesn't have start code and WebRTC OMX decoder implementation behaves incorrectly.

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: Support RTP H.264 input data in WebRTC OMX decoder.

- change configuration code to accept NALU without start code
- add start code to input data before sending to OMX decoder.

Comment 2

[John Lin [:jhlin][:jolin]]

Attachment: Workaround to make jitter buffer correctly deliver SPS/PPS/I-frame.

SPS/PPS/I-frame sharing same timestamp don't work in current jitter buffer code. Change SPS/PPS timestamp to avoid the issue.

Comment 3

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

Comment on attachment 8413678 [details] [diff] [review]
Support RTP H.264 input data in WebRTC OMX decoder.

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

r+ for RTP use with NS_IF_WARN nit fixed.

File followups if/as needed to deal with other start code issues.  If we can turn off start codes (always), we should consider that instead of this patch.

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +44,5 @@
>  #define CODEC_LOGE(...) CSFLogError(LOG_TAG, __VA_ARGS__)
>  
>  namespace mozilla {
>  
> +static uint8_t kNALStartCode[] = { 0x00, 0x00, 0x00, 0x01 };

I take it the OMX decoder only works if Start Codes are included?  Some software codecs have a config that tells it if start codes are required; I know the TI OMX codec has a flag to turn them off:
iPVCapabilityFlags.iOMXComponentUsesNALStartCodes = OMX_FALSE

Is OMX_IndexParamNalStreamFormatSelect (and related options) supported?  See https://www.khronos.org/registry/omxil/extensions/KHR/OpenMAX_IL_1_1_2_Extension%20NAL%20Unit%20Packaging.pdf page 5 (4.3.xx.1)  OMX_NaluFormatOneNaluPerBuffer is probably what we want, if it's supported.  To be generic, the options should be queried and only a supported option should be used (which might be StartCodes; so if StartCodes is always supported we might want to simplify things by always assuming/using StartCodes and stripping or adding them as needed).

If there isn't, that's OK, just an annoyance on decode.  On Encode, we want to make sure we don't use start-codes when packetizing NALs; if the OMX encoder only outputs NALs with start-codes, we'll want to strip them before packetizing (not hard, and can be made a simple 
  if (has_start_code_3_or_4_byte) { modify-ptr-len-passed-to-packetizer }

@@ +314,5 @@
>      // Copying is needed because MediaCodec API doesn't support externallay
>      // allocated buffer as input.
> +    uint8_t* dst = omxIn->data();
> +    memcpy(dst, kNALStartCode, sizeof(kNALStartCode));
> +    memcpy(dst + sizeof(kNALStartCode), aEncoded._buffer, aEncoded._length);

If this code is only to be used for RTP h.264 data (i.e. no start codes), this is fine.  If we plan to use it in the future for data that may have start codes (streaming?) we should test for a start code first (3 or 4-byte!)

@@ +821,5 @@
> +    int32_t height;
> +    status_t result = WebrtcOMXDecoder::ExtractPicDimensions(aInputImage._buffer,
> +                                                             aInputImage._length,
> +                                                             &width, &height);
> +    if (NS_WARN_IF(result != OK)) {

Failing to find SPS/PPS at the start of a stream is not NS_WARN; it's normal operation with packet loss at the start.  You could LOG it, but it should not trigger output unless asked for in a debug build.

Comment 4

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 8413678 [details] [diff] [review]
> Support RTP H.264 input data in WebRTC OMX decoder.
> 
> Review of attachment 8413678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for RTP use with NS_IF_WARN nit fixed.
> 
> File followups if/as needed to deal with other start code issues.  If we can
> turn off start codes (always), we should consider that instead of this patch.
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ +44,5 @@
> >  #define CODEC_LOGE(...) CSFLogError(LOG_TAG, __VA_ARGS__)
> >  
> >  namespace mozilla {
> >  
> > +static uint8_t kNALStartCode[] = { 0x00, 0x00, 0x00, 0x01 };
> 
> I take it the OMX decoder only works if Start Codes are included?  Some
> software codecs have a config that tells it if start codes are required;

I don't know the details of the HW, but the logic of the consumer code is
that it won't try to decode prior to setting this stuff.



> I
> know the TI OMX codec has a flag to turn them off:
> iPVCapabilityFlags.iOMXComponentUsesNALStartCodes = OMX_FALSE
> 
> Is OMX_IndexParamNalStreamFormatSelect (and related options) supported?  See
> https://www.khronos.org/registry/omxil/extensions/KHR/
> OpenMAX_IL_1_1_2_Extension%20NAL%20Unit%20Packaging.pdf page 5 (4.3.xx.1) 
> OMX_NaluFormatOneNaluPerBuffer is probably what we want, if it's supported. 
> To be generic, the options should be queried and only a supported option
> should be used (which might be StartCodes; so if StartCodes is always
> supported we might want to simplify things by always assuming/using
> StartCodes and stripping or adding them as needed).
> 
> If there isn't, that's OK, just an annoyance on decode.  On Encode, we want
> to make sure we don't use start-codes when packetizing NALs; if the OMX
> encoder only outputs NALs with start-codes, we'll want to strip them before
> packetizing (not hard, and can be made a simple 
>   if (has_start_code_3_or_4_byte) { modify-ptr-len-passed-to-packetizer }
> 
> @@ +314,5 @@
> >      // Copying is needed because MediaCodec API doesn't support externallay
> >      // allocated buffer as input.
> > +    uint8_t* dst = omxIn->data();
> > +    memcpy(dst, kNALStartCode, sizeof(kNALStartCode));
> > +    memcpy(dst + sizeof(kNALStartCode), aEncoded._buffer, aEncoded._length);
> 
> If this code is only to be used for RTP h.264 data (i.e. no start codes),
> this is fine.  If we plan to use it in the future for data that may have
> start codes (streaming?) we should test for a start code first (3 or 4-byte!)
> 
> @@ +821,5 @@
> > +    int32_t height;
> > +    status_t result = WebrtcOMXDecoder::ExtractPicDimensions(aInputImage._buffer,
> > +                                                             aInputImage._length,
> > +                                                             &width, &height);
> > +    if (NS_WARN_IF(result != OK)) {
> 
> Failing to find SPS/PPS at the start of a stream is not NS_WARN; it's normal
> operation with packet loss at the start.  You could LOG it, but it should
> not trigger output unless asked for in a debug build.

Comment 5

[John Lin [:jhlin][:jolin]]

Attachment: Support RTP H.264 input data in WebRTC OMX decoder.

Fix nit and carry r+ from jesup.

Comment 6

[John Lin [:jhlin][:jolin]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=b953fd479bd5

Comment 7

[John Lin [:jhlin][:jolin]]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 8413678 [details] [diff] [review]
> Support RTP H.264 input data in WebRTC OMX decoder.
> 
> Review of attachment 8413678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for RTP use with NS_IF_WARN nit fixed.
> 
> File followups if/as needed to deal with other start code issues.  If we can
> turn off start codes (always), we should consider that instead of this patch.
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ +44,5 @@
> >  #define CODEC_LOGE(...) CSFLogError(LOG_TAG, __VA_ARGS__)
> >  
> >  namespace mozilla {
> >  
> > +static uint8_t kNALStartCode[] = { 0x00, 0x00, 0x00, 0x01 };
> 
> I take it the OMX decoder only works if Start Codes are included?  Some
> software codecs have a config that tells it if start codes are required; I
> know the TI OMX codec has a flag to turn them off:
> iPVCapabilityFlags.iOMXComponentUsesNALStartCodes = OMX_FALSE
> 
> Is OMX_IndexParamNalStreamFormatSelect (and related options) supported?  See
> https://www.khronos.org/registry/omxil/extensions/KHR/
> OpenMAX_IL_1_1_2_Extension%20NAL%20Unit%20Packaging.pdf page 5 (4.3.xx.1) 
> OMX_NaluFormatOneNaluPerBuffer is probably what we want, if it's supported. 
> To be generic, the options should be queried and only a supported option
> should be used (which might be StartCodes; so if StartCodes is always
> supported we might want to simplify things by always assuming/using
> StartCodes and stripping or adding them as needed).

 OMX_IndexParamNal* is not supported currently in the code base we have.

> 
> If there isn't, that's OK, just an annoyance on decode.  On Encode, we want
> to make sure we don't use start-codes when packetizing NALs; if the OMX
> encoder only outputs NALs with start-codes, we'll want to strip them before
> packetizing (not hard, and can be made a simple 
>   if (has_start_code_3_or_4_byte) { modify-ptr-len-passed-to-packetizer }

 So far it seems stagefright assumes data in OMX buffer always has start code, so stripping in encoder side and adding in decoder side is needed.

> 
> @@ +314,5 @@
> >      // Copying is needed because MediaCodec API doesn't support externallay
> >      // allocated buffer as input.
> > +    uint8_t* dst = omxIn->data();
> > +    memcpy(dst, kNALStartCode, sizeof(kNALStartCode));
> > +    memcpy(dst + sizeof(kNALStartCode), aEncoded._buffer, aEncoded._length);
> 
> If this code is only to be used for RTP h.264 data (i.e. no start codes),
> this is fine.  If we plan to use it in the future for data that may have
> start codes (streaming?) we should test for a start code first (3 or 4-byte!)

 Eventaully this code will be replaced with platform decoder module adapter that Alfredo is working on (bug 984223). I'd refer handle this problem there.

> 
> @@ +821,5 @@
> > +    int32_t height;
> > +    status_t result = WebrtcOMXDecoder::ExtractPicDimensions(aInputImage._buffer,
> > +                                                             aInputImage._length,
> > +                                                             &width, &height);
> > +    if (NS_WARN_IF(result != OK)) {
> 
> Failing to find SPS/PPS at the start of a stream is not NS_WARN; it's normal
> operation with packet loss at the start.  You could LOG it, but it should
> not trigger output unless asked for in a debug build.

 Will change to CODEC_LOGI() in next version.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/336fe702ad12

Comment 9

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

Comment on attachment 8413681 [details] [diff] [review]
Workaround to make jitter buffer correctly deliver SPS/PPS/I-frame.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +615,5 @@
>        nalu._length = nalSize;
> +      // [Workaround] Jitter buffer code in WebRTC.org cannot correctly deliver
> +      // SPS/PPS/I-frame NALUs that share same timestamp. Change timestamp value
> +      // for SPS/PPS to avoid it.
> +      // TODO: remove when bug 985254 lands. The patch there has same workaround.

Right; I'll likely fix the underlying cause of that when bug 985254 lands so this won't be needed at all

@@ +617,5 @@
> +      // SPS/PPS/I-frame NALUs that share same timestamp. Change timestamp value
> +      // for SPS/PPS to avoid it.
> +      // TODO: remove when bug 985254 lands. The patch there has same workaround.
> +      switch (nalStart[0] & 0x1f) {
> +        case 7:

Since this is temporary, it's ok - normally I'd prefer a #define just to make it readable.  Add a comment about what 7 & 8 mean

Comment 10

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

Landed this for jhlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb62031a7b51

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/336fe702ad12
https://hg.mozilla.org/mozilla-central/rev/cb62031a7b51

Comment 12

[John Lin [:jhlin][:jolin]]

The temporary workaround had a *serious* bug: missing break statement in case 7.
Could you please help land it? Thanks and sorry for the trouble. <(__)>

=== BEGIN ===
# HG changeset patch
# User John Lin <[e-mail]>
Bug 1002402 - add missing break statement.

diff --git a/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp b/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
index 9300469..27addb0 100644
--- a/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
+++ b/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ -615,16 +615,17 @@ private:
       nalu._length = nalSize;
       // [Workaround] Jitter buffer code in WebRTC.org cannot correctly deliver
       // SPS/PPS/I-frame NALUs that share same timestamp. Change timestamp value
       // for SPS/PPS to avoid it.
       // TODO: remove when bug 985254 lands. The patch there has same workaround.
       switch (nalStart[0] & 0x1f) {
         case 7:
           nalu._timeStamp -= 100;
+          break;
         case 8:
           nalu._timeStamp -= 50;
           break;
         default:
           break;
       }
       mCallback->Encoded(nalu, nullptr, nullptr);
     }
=== END ===

Comment 13

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

r+ the fix, landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edd20e5bac2c

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edd20e5bac2c