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)
Tracking
()
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)
Assignee | ||
Comment 2•11 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•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8450208 -
Flags: review?(jolin)
Comment 4•11 years ago
|
||
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+
Comment 6•11 years ago
|
||
[Blocking Requested - why for this release]:
Blocks: CAF-v2.0-CC-metabug
blocking-b2g: --- → 2.0?
Assignee | ||
Comment 7•11 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•11 years ago
|
Whiteboard: [webrtc-uplift]
Comment 8•11 years ago
|
||
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 9•11 years ago
|
||
(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•11 years ago
|
blocking-b2g: 2.0? → 2.0+
Comment 10•11 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Whiteboard: [webrtc-uplift]
You need to log in
before you can comment on or make changes to this bug.
Description
•