Android HW H264: append SPS/PPS at the start of key frame [chromium/src : master]
Weiyong Yao (Gerrit)
Hi watk@, please help to take a look at this. Thanks!
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Chris Watkins (Gerrit)
Chris Watkins would like Frank Liberato to review this change.
Android HW H264: append SPS/PPS at the start of key frame
Android MediaCodec will generate a seperate config frame containing
SPS/PPS at beginning of encoding H264 stream and the following key
frames won't have SPS/PPS any more, while other SW/HW H264 encoder
implementations will generate key frames with SPS/PPS always.
Recently WebRTC requires the H264 IDR/keyframe to contain SPS/PPS to
start decoder. Then the H264 intercommunication is broken between
Android and other platforms.
To fix this, append SPS/PPS at the start of H264 keyframe on Android.
Bug:761336
Change-Id: Ifb6b27f4448186dc4d7411921c556cf57d74068e
---
M media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
1 file changed, 90 insertions(+), 17 deletions(-)
Chris Watkins (Gerrit)
I took a look but I'll pass the final review to liberato@ as I've moved teams now.
6 comments:
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:
Patch Set #1, Line 412:
getOutputBuffersWhy are we no longer using our mOutputBuffer array?
Is there a reason this no longer catches IllegalState?
Patch Set #1, Line 481:
codecOutputBufferDeclaration can move closer to usage.
Patch Set #1, Line 482:
OutputBufferSizeNaming style not consistent here
Patch Set #1, Line 518:
dequeueOutputBufferDoes this need to update mLastPresentationTimeUs?
Patch Set #1, Line 553:
codecOutputBufferI'm worried about doubling the memory consumed by output buffers. Wouldn't it be better to keep a reference to the ByteBuffer that MediaCodec#getOutputBuffer() returns, maybe along with the offset, size, and config?
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Weiyong Yao (Gerrit)
Thanks watk@! All comments are addressed or answered.
liberato@, please take a look. Thanks!
6 comments:
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:
Patch Set #1, Line 412:
NT <= Build.VERS
Why are we no longer using our mOutputBuffer array?
Done
Patch Set #1, Line 418:
throw new IllegalStateException("Got null output buffer");
Is there a reason this no longer catches IllegalState?
This function is a private one now and will be called by queueOutputBuffer() only with a try-catch around, which will handle the exception.
Added a comment for it.Patch Set #1, Line 481:
= MediaCodecStatu
Declaration can move closer to usage.
Done
Patch Set #1, Line 482:
outputBufferSize
Naming style not consistent here
Done
Patch Set #1, Line 518:
the frame has a wr
Does this need to update mLastPresentationTimeUs?
Done
Patch Set #1, Line 553:
position(outputBuI'm worried about doubling the memory consumed by output buffers. […]
Sorry I didn't get you. Could you please help to elaborate it?
The purpose is to append configData at the beginning of a key frame (appending config to the end of key frame won't work). Using two Bytebuffer may be the simplest method, I suppose.
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
sorry for the delay.
at a high level, this seems like it's adding some very specific functionality to MediaCodecBridge, that might be specific to WebRTC. it also might affect the decoder cases, with all the changes to how output buffers are managed. at least, it's not immediately obvious to me that it doesn't. :)
is there another option, like exposing the few things (is_config_frame), and handling all of it in a wrapper class? might even be a native wrapper class, since DequeueOutputInfo makes it that far and could be expanded to include whatever other fields you need to expose.
thanks
-fl
Weiyong Yao (Gerrit)
Patch Set 2:
sorry for the delay.
at a high level, this seems like it's adding some very specific functionality to MediaCodecBridge, that might be specific to WebRTC. it also might affect the decoder cases, with all the changes to how output buffers are managed. at least, it's not immediately obvious to me that it doesn't. :)
is there another option, like exposing the few things (is_config_frame), and handling all of it in a wrapper class? might even be a native wrapper class, since DequeueOutputInfo makes it that far and could be expanded to include whatever other fields you need to expose.
thanks
-fl
Yes I was bothered by the fact the MediaCodecBridge serves multiple functionalities too, video encoder/video decoder/audio decoder. I did think about to do the appending job in android_video_encode_accelerator.cc or media_codec_bridge_impl.cc. (Even I wanted to split the MediaCodecBridge.java into several files, which is not trivial though :) )
But it's still a better idea to do the appending stuff in MediaCodecBridge.java.
It's encoder's job to have SPS/PPS in IDR/Keyframe according to the spec, as all other H264 encoder implementations (SW/HW) on other platforms do, except MediaCodec on Android. So it's a more natural thinking to do the job in encoder implementation before outputting to native. Also people will realize this problem more easily by checking this java file, comparing with other encoder implementations.
So I prefer to get it done in java level. The affection to decoding side is almost zero, expect dequeuing output into local buffers first.
Please let me know if you have more concerns.
Frank Liberato (Gerrit)
It's encoder's job to have SPS/PPS in IDR/Keyframe according to the spec
have you opened a bug with android? if not, please consider doing so.
if they do fix it in a future release of android, then this code gets more complicated -- we'll want to be able to turn this back off at runtime depending on the version.
i'm fairly sure that we want to keep the java side as simple as possible, especially for the common case. if we start forcing MediaCodec to transfer decoded frames back to chrome, performance won't be so good.
is it possible to subclass / wrap the java side just for the encoder? that might avoid much of the complexity you mention with the native side, but still keep the decoder path simple.
WDYT?
thanks
-fl
-fl
2 comments:
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:
Patch Set #2, Line 528:
codecOutputBuffer = getMediaCodecOutputBuffer(index);i'm not at all sure that this is safe for decoding. we usually connect MediaCodec to an Android Surface, and never touch the decoded data directly. we just move the index around.
doing this might cause all sorts of copying of data. i base that on some work i did a year or so ago, where i tried to do something with the buffer data. that's more than just the copy that you're making explicitly below.
plus, it's almost certainly not going to work as intended if this is a secure decoder (e.g., for protected content).
Patch Set #2, Line 555:
frameBuffer.put(codecOutputBuffer);this can be an enormous amount of data.
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Weiyong Yao (Gerrit)
Patch Set 2:
(2 comments)
It's encoder's job to have SPS/PPS in IDR/Keyframe according to the spec
have you opened a bug with android? if not, please consider doing so.
if they do fix it in a future release of android, then this code gets more complicated -- we'll want to be able to turn this back off at runtime depending on the version.
i'm fairly sure that we want to keep the java side as simple as possible, especially for the common case. if we start forcing MediaCodec to transfer decoded frames back to chrome, performance won't be so good.
is it possible to subclass / wrap the java side just for the encoder? that might avoid much of the complexity you mention with the native side, but still keep the decoder path simple.
WDYT?
thanks
-fl-fl
Thanks so much for the comments!
Totally agree with you. I'll see if I can move encoder part into a separate file in this cl. And if needed, split encoder and decode completely in a follow cl. WDYT?
Weiyong Yao (Gerrit)
Hi liberato@, please help to check if this is in the right direction.
I subclass MediaCodecEncoder out of MedicaCodecBright and override several methods needed to restore H264 for webRTC.
This can be a good base to make MediaCodecEncoder totally independent. If it's the plan, I can file an issue and start looking into it in a separate cl soon.
Frank Liberato (Gerrit)
Patch Set 4:
Hi liberato@, please help to check if this is in the right direction.
thanks for taking the time with this. i think that it's moving in the right direction. there's quite a bit of duplicate code that can probably be shared between Bridge and Encoder.
This can be a good base to make MediaCodecEncoder totally independent.
i'm not sure that we want them to be independent. almost all of the encoder would be a duplicate of the decoder. MediaCodecBridge just needs to allow subclasses to override how buffers are handled, and let the subclass check for config frames.
thanks
-fl
3 comments:
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:
Patch Set #4, Line 88:
public"protected", to give access to subclasses but not others. here and elsewhere.
File media/base/android/java/src/org/chromium/media/MediaCodecEncoder.java:
Patch Set #4, Line 42:
private static class EncoderDequeueOutputResult extends DequeueOutputResult {why does this need to be subclassed?
if (indexOrStatus >= 0) {
boolean isConfigFrame = (info.flags & MediaCodec.BUFFER_FLAG_CODEC_CONFIG) != 0;
if (isConfigFrame) {
Log.i(TAG,
"Config frame generated. Offset: " + info.offset
+ ". Size: " + info.size);
codecOutputBuffer = getMediaCodecOutputBuffer(indexOrStatus);
codecOutputBuffer.position(info.offset);
codecOutputBuffer.limit(info.offset + info.size);
mConfigData = ByteBuffer.allocateDirect(info.size);
mConfigData.put(codecOutputBuffer);
// Log few SPS header bytes to check profile and level.
StringBuilder spsData = new StringBuilder();
for (int i = 0; i < (info.size < 8 ? info.size : 8); i++) {
spsData.append(Integer.toHexString(mConfigData.get(i) & 0xff)).append(" ");
}
Log.i(TAG, spsData.toString());
mConfigData.rewind();
// Release buffer back.
mMediaCodec.releaseOutputBuffer(indexOrStatus, false);
// Query next output.
indexOrStatus = mMediaCodec.dequeueOutputBuffer(info, timeoutUs);
}
}there's a lot of duplicated code here. would it be possible to make the encoder subclass much lighter?
for example, we might have protected methods to handle just this part, that MediaCodecBridge.dequeueOutputBuffer calls. the default implementation in MediaCodecBridge would be empty. something like processConfigFrame(), that's called right after mMediaCodec.dequeueOutputBuffer if the index >= 0.
the encoder subclass would do buffer management like it does now, as well.
then the encoder is a very small class.
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Weiyong Yao (Gerrit)
I moved lots of codes in previous patch because I was still thinking about a separate encoder class. Since that's not the plan, the MediaCodecEncoder can be much lighter. Please take a look at the latest patch. Thanks!
3 comments:
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:
Patch Set #4, Line 88:
private
"protected", to give access to subclasses but not others. here and elsewhere.
Done
File media/base/android/java/src/org/chromium/media/MediaCodecEncoder.java:
Patch Set #4, Line 42:
protected void releaseOutputBuffer(int index, boolean render) {
why does this need to be subclassed?
I was thinking about moving more codes into MediaCodecEncoder, for the proposed separation.
Since we don't want to do it, there is no need to override this and dequeueOutputBuffer any more.
there's a lot of duplicated code here. […]
Done
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
this is coming along nicely. overall approach seems good.
-fl
8 comments:
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:
Patch Set #6, Line 214:
if (direction == MediaCodecDirection.ENCODER) {should you do this just if it's H264? then you might call the new class MediaCodecH264Encoder, and save the copies / checking mime types / etc.
File media/base/android/java/src/org/chromium/media/MediaCodecEncoder.java:
Patch Set #6, Line 19:
encoding processingplease add some detail about why.
Patch Set #6, Line 25:
mOutputBuffersplease comment what this is.
Patch Set #6, Line 33:
mOutputBuffers = new SparseArray<>();can be moved to @25, or not.
Patch Set #6, Line 62:
if (isConfigFrame) {should this also check for H264, so that it's not bothering to copy config frames that it won't use?
Patch Set #6, Line 108:
frameBuffer.put(codecOutputBuffer);rather than doing the copy if it's not a keyframe or if it's not h264, could you just put codecOutputBuffer into mOutputBuffers?
try {
indexOrStatus = mMediaCodec.dequeueOutputBuffer(info, timeoutUs);
ByteBuffer codecOutputBuffer = null;
if (indexOrStatus >= 0) {
boolean isConfigFrame = (info.flags & MediaCodec.BUFFER_FLAG_CODEC_CONFIG) != 0;
if (isConfigFrame) {
Log.d(TAG, "Config frame generated. Offset: %d, size: %d", info.offset,
info.size);
codecOutputBuffer = getMediaCodecOutputBuffer(indexOrStatus);
codecOutputBuffer.position(info.offset);
codecOutputBuffer.limit(info.offset + info.size);
mConfigData = ByteBuffer.allocateDirect(info.size);
mConfigData.put(codecOutputBuffer);
// Log few SPS header bytes to check profile and level.
StringBuilder spsData = new StringBuilder();
for (int i = 0; i < (info.size < 8 ? info.size : 8); i++) {
spsData.append(Integer.toHexString(mConfigData.get(i) & 0xff)).append(" ");
}
Log.i(TAG, "spsData: %s", spsData.toString());
mConfigData.rewind();
// Release buffer back.
mMediaCodec.releaseOutputBuffer(indexOrStatus, false);
// Query next output.
indexOrStatus = mMediaCodec.dequeueOutputBuffer(info, timeoutUs);
}
}
if (indexOrStatus >= 0) {
codecOutputBuffer = getMediaCodecOutputBuffer(indexOrStatus);
codecOutputBuffer.position(info.offset);
codecOutputBuffer.limit(info.offset + info.size);
// Check key frame flag.
boolean isKeyFrame = (info.flags & MediaCodec.BUFFER_FLAG_SYNC_FRAME) != 0;
if (isKeyFrame) {
Log.d(TAG, "Key frame generated");
}
final ByteBuffer frameBuffer;
if (isKeyFrame && mMime.equals(MimeTypes.VIDEO_H264)) {
Log.d(TAG, "Appending config frame of size %d to output buffer with size %d",
mConfigData.capacity(), info.size);
// For H.264 encoded key frame append SPS and PPS NALs at the start.
frameBuffer = ByteBuffer.allocateDirect(mConfigData.capacity() + info.size);
frameBuffer.put(mConfigData);
info.offset = 0;
info.size += mConfigData.capacity();
} else {
frameBuffer = ByteBuffer.allocateDirect(info.offset + info.size);
frameBuffer.position(info.offset);
}
frameBuffer.put(codecOutputBuffer);
frameBuffer.rewind();
mOutputBuffers.put(indexOrStatus, frameBuffer.duplicate());
}
} catch (IllegalStateException e) {
Log.e(TAG, "Failed to dequeue output buffer", e);
}this could use a few more high-level comments.
Patch Set #6, Line 121:
private ByteBuffer getMediaCodecOutputBuffer(int index) {is this different than "return super.getOutputBuffer(index)", maybe with a throw?
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Weiyong Yao (Gerrit)
All comments are addressed/answered. PTAL!
8 comments:
Patch Set #6, Line 214:
if (direction == MediaCodecDirection.ENCODER) {
should you do this just if it's H264? then you might call the new class MediaCodecH264Encoder, and […]
Not sure about this. Currently only H264 encoder is enabled on Android.
VPx encoder may be enabled in the future. So far it's hard to tell whether any tweaking will be added or not for VPx encoder. Maybe it's better to keep the new subclass for all encoders. WDYT?
File media/base/android/java/src/org/chromium/media/MediaCodecEncoder.java:
Patch Set #6, Line 19:
R/keyframe should h
please add some detail about why.
Done
Patch Set #6, Line 25:
aCodecBridge {
please comment what this is.
Done
Patch Set #6, Line 33:
protected MediaCodecEncoder(MediaCodec mediaCodec, String mime,
can be moved to @25, or not.
Done
Patch Set #6, Line 62:
ByteBuffer codecOutputBuffer = null;
should this also check for H264, so that it's not bothering to copy config frames that it won't use?
Done
Patch Set #6, Line 108:
info.size += mConfigData.capacity();rather than doing the copy if it's not a keyframe or if it's not h264, could you just put codecOutpu […]
Done
ected int dequeueOutputBufferInternal(MediaCodec.BufferInfo info, long timeoutUs) {
int indexOrStatus = -1;
try {
indexOrStatus = mMediaCodec.dequeueOutputBuffer(info, timeoutUs);
ByteBuffer codecOutputBuffer = null;
if (indexOrStatus >= 0) {
boolean isConfigFrame = (info.flags & MediaCodec.BUFFER_FLAG_CODEC_CONFIG) != 0;
if (isConfigFrame && mMime.equals(MimeTypes.VIDEO_H264)) {
frameBuffer.put(codecOutputBuffer);
frameBuffer.rewind();
info.offset = 0;
info.size += mConfigData.capacity();
mOutputBuffers.put(indexOrStatus, frameBuffer.duplicate());
} else {
mOutputBuffers.put(indexOrStatus, codecOutputBuffer.duplicate());
}
}
}
this could use a few more high-level comments.
Done
Patch Set #6, Line 121:
// Call this function with catching IllegalStateException.is this different than "return super. […]
Done
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
one other thing: is it possible to extend media_codec_bridge_unittest to include a test for this behavior? it already does something quite similar for decoding.
thanks
-fl
3 comments:
Patch Set #6, Line 214:
if (direction == MediaCodecDirection.ENCODER) {
Not sure about this. Currently only H264 encoder is enabled on Android. […]
that's a good question.
as it's written, the subclass is specifically checking for H264 anyway. it's doing nothing functionally differently than MediaCodecBridge unless the format matches. i don't think there's an advantage to running the extra code for any other format given that those guards are there.
i think that we should go all the way to one side or the other. my suggestion is to remove the format guards in the encoder subclass, and then either (a) instantiate the new encoder only for H264, or (b) instantiate the new encoder for all cases when we're encoding.
i'll agree with whichever you think is the right thing to do. is it safe to "if we get a config frame, then prepend SPS/PPS"?
File media/base/android/java/src/org/chromium/media/MediaCodecEncoder.java:
Patch Set #7, Line 109:
duplicate()since this is a new buffer that's not used anymore, i think that you don't need to duplicate() here.
please note that i might misunderstand what duplicate() does, so i might simply be making bad suggestions. :)
Patch Set #7, Line 111:
duplicate()what happens if one doesn't call duplicate()? this one seems more complicated than @109.
is duplicate() just to provide a different "current offset" to two holders of the object, sharing the same underlying data?
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Weiyong Yao (Gerrit)
Thanks for the reminding! I only noticed there are test cases for audio decoding in media_codec_bridge_impl_unittest.cc in the past. Good to find the case for video decoding.
I'm headache on how to add auto tests for HW H264 on Android now. Extending media_codec_bridge_impl_unittest.cc is a good start. Can I do it in a following cl (just filed https://crbug.com/776122 for it) soon?
3 comments:
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:
Patch Set #6, Line 214:
// Create MediaCodecEncoder for H264 to meet WebRTC requirements to IDR/keyframes.that's a good question. […]
Done
OK. Create the new encoder for H264 for now. Will revisit when other hw encoder will be enabled.
PS: I guess it's safe(and necessary) to "if we get a config frame, then prepend SPS/PPS" to VP9/H265 too. VP8 won't have config frame.
File media/base/android/java/src/org/chromium/media/MediaCodecEncoder.java:
since this is a new buffer that's not used anymore, i think that you don't need to duplicate() here. […]
Done
sorry, it's my misunderstanding to duplicate() here.
what happens if one doesn't call duplicate()? this one seems more complicated than @109. […]
Done
To view, visit change 710454. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Can I do it in a following cl
yes.
lgtm.
thanks
-fl
Patch set 8:Code-Review +1
Weiyong Yao (Gerrit)
Patch set 8:Commit-Queue +2
Commit Bot (Gerrit)
Try jobs failed on following builders:
win7_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/21777)
Weiyong Yao (Gerrit)
Commit Bot (Gerrit)
Commit Bot merged this change.
Android HW H264: append SPS/PPS at the start of key frame
Android MediaCodec will generate a seperate config frame containing
SPS/PPS at beginning of encoding H264 stream and the following key
frames won't have SPS/PPS any more, while other SW/HW H264 encoder
implementations will generate key frames with SPS/PPS always.
Recently WebRTC requires the H264 IDR/keyframe to contain SPS/PPS to
start decoder. Then the H264 intercommunication is broken between
Android and other platforms.
To fix this, append SPS/PPS at the start of H264 keyframe on Android.
Bug: 761336
Change-Id: Ifb6b27f4448186dc4d7411921c556cf57d74068e
Reviewed-on: https://chromium-review.googlesource.com/710454
Reviewed-by: Frank Liberato <libe...@chromium.org>
Commit-Queue: Weiyong Yao <brav...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510459}
---
M media/base/android/BUILD.gn
M media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
A media/base/android/java/src/org/chromium/media/MediaCodecEncoder.java
3 files changed, 150 insertions(+), 8 deletions(-)