Closed Bug 1030112 Opened 10 years ago Closed 10 years ago

8x10 OMX H.264 encoder timestamps don't get passed through to encode result

Categories

(Core :: WebRTC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(1 file)

On issue we worked around for 2.0 was a problem with timestamps in the encoder output.  There were a number of emails back and forth, and we worked around the issue in WebrtcOMXH264VideoCodec using #ifdef OMX_OUTPUT_TIMESTAMPS_WORK.  We weren't getting the same output timestamps when we dequeue encoded frames as we passed into the encoder.

This bug is to cover resolving that in the underlying code/firmware and removing the workaround.
We looked at this issue and found two problem in WebrtcOMXH264VideoCodec.cpp

1. There is a typo in the equation that converts the RTP timestamp in WebrtcOMXH264VideoCodec.cpp.
2. Multiple 1000/90 generates non integer and it truncates the value. This needs to be considered when re-calculating target_timestamp.

Please, see if the following change makes sense.  

--- a/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
+++ b/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ -42,6 +42,7 @@ using namespace android;
 #define CODEC_LOGI(...) CSFLogInfo(LOG_TAG, __VA_ARGS__)
 #define CODEC_LOGW(...) CSFLogWarn(LOG_TAG, __VA_ARGS__)
 #define CODEC_LOGE(...) CSFLogError(LOG_TAG, __VA_ARGS__)
+#define OMX_OUTPUT_TIMESTAMPS_WORK 1
 
 namespace mozilla {
 
@@ -591,7 +592,7 @@ protected:
       return false;
     }
 
-    uint32_t target_timestamp = (timeUs * 90ll) / 1000; // us -> 90KHz
+    uint32_t target_timestamp = ceil((double)(timeUs / 1000.0) * 90.0); // us -> 90KHz
     bool isParamSets = (flags & MediaCodec::BUFFER_FLAG_CODECCONFIG);
     bool isIFrame = (flags & MediaCodec::BUFFER_FLAG_SYNCFRAME);
     CODEC_LOGD("OMX: encoded frame (%d): time %lld (%u), flags x%x",
@@ -887,13 +888,13 @@ WebrtcOMXH264VideoEncoder::Encode(const webrtc::I420VideoFrame& aInputImage,
 
   CODEC_LOGD("Encode frame: %dx%d, timestamp %u (%lld), renderTimeMs %u",
              aInputImage.width(), aInputImage.height(),
-             aInputImage.timestamp(), aInputImage.timestamp() * 100011 / 90,
+             aInputImage.timestamp(), aInputImage.timestamp() * 1000ll / 90,
              aInputImage.render_time_ms());
 
   nsresult rv = mOMX->Encode(&img,
                              yuvData.mYSize.width,
                              yuvData.mYSize.height,
-                             aInputImage.timestamp() * 100011 / 90, // 90kHz -> us.
+                             aInputImage.timestamp() * 1000ll/ 90, // 90kHz -> us.
                              0);
   if (rv == NS_OK) {
     if (mOutputDrain == nullptr) {
Flags: needinfo?(rjesup)
Thanks!  The wonders of typos and fonts...  Good find.

I changed the floating-point to fixed-point round-up (the rounding you were doing with ceil() to offset the round-down when calling Encode()).

The only issue remaining is that the first frame encoded in each call (or encoder init) generates SPS/PPS frame with a 0 timestamp, instead of the timestamp of the first input frame.

Will that happen on all SPS/PPS generated, including after a reconfig to a new resolution? (I note that a reconfig for a new framerate doesn't generate SPS/PPS; on a resolution change that would clearly be required).

In any case, since I know it's SPS/PPS, I can give if the timestamp of the oldest frame in the encoder's queue and not pop that entry.
Flags: needinfo?(rjesup)
Attachment #8450208 - Flags: review?(jolin)
Comment on attachment 8450208 [details] [diff] [review]
Fix typo in OMX H264 encode timestamp matching and deal with SPS/PPS timestamp assignment

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

LGTM.
P.S., had to change browser monospace font to Inconsolata to actually see the distinction between lowercase L and number 1. XD
Attachment #8450208 - Flags: review?(jolin) → review+
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Somehow the patch disappeared from my queues...  Landed now; worth considering for 2.0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a35174bd170
Target Milestone: --- → mozilla34
Whiteboard: [webrtc-uplift]
https://hg.mozilla.org/mozilla-central/rev/5a35174bd170
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to Randell Jesup [:jesup] from comment #7)
> Landed now; worth considering for 2.0

FWIW, we've had this patch in our v2.0 tree for over a month now.
blocking-b2g: 2.0? → 2.0+
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: