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

RESOLVED FIXED in Firefox 34, Firefox OS v2.0

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Blocks: 1 bug)

Trunk
mozilla34
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.

Comment 1

4 years ago
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)
(Assignee)

Comment 2

4 years ago
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)
(Assignee)

Comment 3

4 years ago
Created attachment 8450208 [details] [diff] [review]
Fix typo in OMX H264 encode timestamp matching and deal with SPS/PPS timestamp assignment
(Assignee)

Updated

4 years ago
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+
Duplicate of this bug: 1035105

Updated

4 years ago
Blocks: 996859
[Blocking Requested - why for this release]:
Blocks: 1041241
blocking-b2g: --- → 2.0?
(Assignee)

Comment 7

4 years ago
Somehow the patch disappeared from my queues...  Landed now; worth considering for 2.0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a35174bd170
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → affected
Target Milestone: --- → mozilla34
(Assignee)

Updated

4 years ago
Whiteboard: [webrtc-uplift]

Comment 8

4 years ago
https://hg.mozilla.org/mozilla-central/rev/5a35174bd170
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Last Resolved: 4 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.

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2956b28771c0
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
status-firefox34: affected → fixed
(Assignee)

Updated

4 years ago
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.