Closed Bug 1030112 Opened 11 years ago Closed 11 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]
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 11 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: