This tool demonstrates the use of the Media Foundation H.264 decoder as a sta...

This tool demonstrates the use of the Media Foundation H.264 decoder as a standalone Media Foundation Transform (MFT). See README for more information.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54078

[imcheng, 2010-07-23 17:59:46]

This tool decodes H.264 video by using the Media Foundation H.264 decoder as a
standalone. It currently uses ffmpeg's parser to read from a mp4 file and
ffmpeg's converter to convert to annex b stream.

It can do both software and hardware decoding. (--enable-dxva to enable hardware
decoding).

[Alpha Left Google, 2010-07-23 20:36:57]

http://codereview.chromium.org/3044019/diff/1/6
File media/tools/h264mft/h264mft.cc (right):

http://codereview.chromium.org/3044019/diff/1/6#newcode16
media/tools/h264mft/h264mft.cc:16: #include <algorithm>
STL includes should come first.

http://codereview.chromium.org/3044019/diff/1/6#newcode75
media/tools/h264mft/h264mft.cc:75: IMFSample*
ConvertAnnexBStreamToMFSample(uint8* stream, int size,
The conversion doesn't actually happen in this method, maybe just call it
CreateInputSample()?

http://codereview.chromium.org/3044019/diff/1/6#newcode161
media/tools/h264mft/h264mft.cc:161: bool H264Mft::SendInput(IMFSample* sample) {
You can make this method private, because user won't deal with IMFSample
directly. I think you can merge this with SendInput(unit8* stream...);

http://codereview.chromium.org/3044019/diff/1/6#newcode231
media/tools/h264mft/h264mft.cc:231: LOG(INFO) << "ProcessOutput failed with
status " << std::hex << hr
This block of code should be moved to a helper method for converting the error
message.

http://codereview.chromium.org/3044019/diff/1/6#newcode326
media/tools/h264mft/h264mft.cc:326: return false;
nit: indentation

http://codereview.chromium.org/3044019/diff/1/6#newcode333
media/tools/h264mft/h264mft.cc:333: return false;
nit: need to use {} to enclose this.

http://codereview.chromium.org/3044019/diff/1/7
File media/tools/h264mft/h264mft.h (right):

http://codereview.chromium.org/3044019/diff/1/7#newcode32
media/tools/h264mft/h264mft.h:32: IMFTransform* GetH264Decoder();
This should be static in the .cc file. Remove it from the header file.

http://codereview.chromium.org/3044019/diff/1/7#newcode35
media/tools/h264mft/h264mft.h:35: IMFSample* CreateEmptySample();
This is static too.

http://codereview.chromium.org/3044019/diff/1/7#newcode38
media/tools/h264mft/h264mft.h:38: IMFSample* CreateEmptySampleWithBuffer(int
buffer_length);
This is also static.

http://codereview.chromium.org/3044019/diff/1/7#newcode45
media/tools/h264mft/h264mft.h:45: IMFSample*
ConvertAnnexBStreamToMFSample(uint8* stream, int size,
Make this static also.

http://codereview.chromium.org/3044019/diff/1/7#newcode52
media/tools/h264mft/h264mft.h:52: enum DecoderOutputState {
Chromium uses the style of kOutputOk, etc for enum style.

http://codereview.chromium.org/3044019/diff/1/7#newcode77
media/tools/h264mft/h264mft.h:77: bool SendInput(IMFSample* sample);
Try to hide media foundation types in the class, so I'd remove this.

http://codereview.chromium.org/3044019/diff/1/7#newcode84
media/tools/h264mft/h264mft.h:84: bool SendInput(uint8* stream, int size, int64
duration, int64 sample_time);
s/stream/data/
s/sample_time/timestamp/

Put timestamp before duration.

Also explain what is the return value.

http://codereview.chromium.org/3044019/diff/1/7#newcode92
media/tools/h264mft/h264mft.h:92: bool SendDrainMessage();
What is the return value?

http://codereview.chromium.org/3044019/diff/1/7#newcode92
media/tools/h264mft/h264mft.h:92: bool SendDrainMessage();
What if user seeks to a certain position after the video is ended? What kind of
signal do we need to send to the decoder?

We don't need to solve this problem now but just want to kind in mind.

http://codereview.chromium.org/3044019/diff/1/7#newcode97
media/tools/h264mft/h264mft.h:97: int in_buffer_size() const { return
in_buffer_size_; }
What is in_buffer_size() and out_buffer_size()?

[imcheng, 2010-07-26 20:12:49]

I've added a new patch so that H264Mft::GetOutput() now returns a
media::VideoFrame object. In the next patch I will work on making it a
pull-based component using callbacks.

http://codereview.chromium.org/3044019/diff/1/6
File media/tools/h264mft/h264mft.cc (right):

http://codereview.chromium.org/3044019/diff/1/6#newcode16
media/tools/h264mft/h264mft.cc:16: #include <algorithm>
On 2010/07/23 20:36:57, Alpha wrote:
> STL includes should come first.

For some reason lint thinks the mf libraries are C system libraries so they
should appear first before the STL ones. But I will change it.

http://codereview.chromium.org/3044019/diff/1/6#newcode75
media/tools/h264mft/h264mft.cc:75: IMFSample*
ConvertAnnexBStreamToMFSample(uint8* stream, int size,
On 2010/07/23 20:36:57, Alpha wrote:
> The conversion doesn't actually happen in this method, maybe just call it
> CreateInputSample()?
> 

Done.

http://codereview.chromium.org/3044019/diff/1/6#newcode161
media/tools/h264mft/h264mft.cc:161: bool H264Mft::SendInput(IMFSample* sample) {
On 2010/07/23 20:36:57, Alpha wrote:
> You can make this method private, because user won't deal with IMFSample
> directly. I think you can merge this with SendInput(unit8* stream...);
> 

Done.

http://codereview.chromium.org/3044019/diff/1/6#newcode231
media/tools/h264mft/h264mft.cc:231: LOG(INFO) << "ProcessOutput failed with
status " << std::hex << hr
On 2010/07/23 20:36:57, Alpha wrote:
> This block of code should be moved to a helper method for converting the error
> message.
> 

Done.

http://codereview.chromium.org/3044019/diff/1/6#newcode326
media/tools/h264mft/h264mft.cc:326: return false;
On 2010/07/23 20:36:57, Alpha wrote:
> nit: indentation

Done.

http://codereview.chromium.org/3044019/diff/1/6#newcode333
media/tools/h264mft/h264mft.cc:333: return false;
On 2010/07/23 20:36:57, Alpha wrote:
> nit: need to use {} to enclose this.

Done.

http://codereview.chromium.org/3044019/diff/1/7
File media/tools/h264mft/h264mft.h (right):

http://codereview.chromium.org/3044019/diff/1/7#newcode32
media/tools/h264mft/h264mft.h:32: IMFTransform* GetH264Decoder();
On 2010/07/23 20:36:57, Alpha wrote:
> This should be static in the .cc file. Remove it from the header file.
> 

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode35
media/tools/h264mft/h264mft.h:35: IMFSample* CreateEmptySample();
On 2010/07/23 20:36:57, Alpha wrote:
> This is static too.

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode38
media/tools/h264mft/h264mft.h:38: IMFSample* CreateEmptySampleWithBuffer(int
buffer_length);
On 2010/07/23 20:36:57, Alpha wrote:
> This is also static.

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode45
media/tools/h264mft/h264mft.h:45: IMFSample*
ConvertAnnexBStreamToMFSample(uint8* stream, int size,
On 2010/07/23 20:36:57, Alpha wrote:
> Make this static also.

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode52
media/tools/h264mft/h264mft.h:52: enum DecoderOutputState {
On 2010/07/23 20:36:57, Alpha wrote:
> Chromium uses the style of kOutputOk, etc for enum style.

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode77
media/tools/h264mft/h264mft.h:77: bool SendInput(IMFSample* sample);
On 2010/07/23 20:36:57, Alpha wrote:
> Try to hide media foundation types in the class, so I'd remove this.

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode84
media/tools/h264mft/h264mft.h:84: bool SendInput(uint8* stream, int size, int64
duration, int64 sample_time);
On 2010/07/23 20:36:57, Alpha wrote:
> s/stream/data/
> s/sample_time/timestamp/
> 
> Put timestamp before duration.
> 
> Also explain what is the return value.

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode92
media/tools/h264mft/h264mft.h:92: bool SendDrainMessage();
On 2010/07/23 20:36:57, Alpha wrote:
> What is the return value?
> 

Done.

http://codereview.chromium.org/3044019/diff/1/7#newcode92
media/tools/h264mft/h264mft.h:92: bool SendDrainMessage();
SendStartMessage() should be called again. This sends a
MFT_MESSAGE_NOTIFY_START_OF_STREAM message to the MFT.

On 2010/07/23 20:36:57, Alpha wrote:
> What if user seeks to a certain position after the video is ended? What kind
of
> signal do we need to send to the decoder?
> 
> We don't need to solve this problem now but just want to kind in mind.

http://codereview.chromium.org/3044019/diff/1/7#newcode97
media/tools/h264mft/h264mft.h:97: int in_buffer_size() const { return
in_buffer_size_; }
Done. These are minimum input and output buffer sizes as required by decoder.

On 2010/07/23 20:36:57, Alpha wrote:
> What is in_buffer_size() and out_buffer_size()?
>

[Alpha Left Google, 2010-07-27 18:01:47]

LGTM with additional small change.

I think the file should be called mft_h264_decoder.cc so the class will be
MftH264Decoder, but that can be done in the next patch.

http://codereview.chromium.org/3044019/diff/17001/18001
File media/base/video_frame.h (right):

http://codereview.chromium.org/3044019/diff/17001/18001#newcode36
media/base/video_frame.h:36: NV12,        // 12bpp YVU planar 1x1 Y, 2x2 UV
interleaving samples
Please update remoting/base/protocol/chromotocol.proto to reflect changes to the
color space format.

[imcheng, 2010-07-28 00:25:44]

I changed the decoder to use callbacks. It is a little odd though because it is
still synchronous. I also renamed the file and class in this patch.

http://codereview.chromium.org/3044019/diff/17001/18001
File media/base/video_frame.h (right):

http://codereview.chromium.org/3044019/diff/17001/18001#newcode36
media/base/video_frame.h:36: NV12,        // 12bpp YVU planar 1x1 Y, 2x2 UV
interleaving samples
On 2010/07/27 18:01:47, Alpha wrote:
> Please update remoting/base/protocol/chromotocol.proto to reflect changes to
the
> color space format.
> 

Done.

[Evan Martin, 2010-07-29 00:49:22]

This appeared to break the build:
http://build.chromium.org/buildbot/waterfall/builders/Chromium%20XP/builds/13365

So I reverted it.  Feel free to land it again if you've fixed it.
In the future if you'd like to protest your case it'd help to be on IRC.  :)

[Lei Zhang, 2010-07-29 03:18:52]

Also, you've checked in files with the executable permission set. If you're on
Windows, see http://code.google.com/p/chromium/wiki/UsingGit#Windows