Add media file validation to utility process

Add media file validation to utility process

This does not do a full validation of the media file, but only does basic checks.

BUG=144509
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216653

[vandebo (ex-Chrome), 2013-08-07 06:12:36]

dalecurtis for media/* and chrome/utility/DEPS
tsepez for chrome_utility_message.h
thestig for chrome/*

[vandebo (ex-Chrome), 2013-08-07 06:13:22]

BTW - https://codereview.chromium.org/20190002/ is the consumer of this change.

[Tom Sepez, 2013-08-07 16:49:16]

Messages LGTM.

[DaleCurtis, 2013-08-07 18:27:06]

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:21: void ClearBoolCallback(bool* called) {
Name is pretty meaningless.  How about OnError() instead.  Also make it static.

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:45: bool MediaFileChecker::Start(const
base::TimeDelta& check_time) {
Sanity check check_time to be positive and less than some maximum?

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:50: base::Unretained(&read_ok)));
Is unretained necessary here?

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:63: for (size_t i = 0; i <
format_context->nb_streams; ++i) {
Just FYI, the way you're doing this doesn't verify that the file is what it says
it is, just that we can decode it.  Also, it ignores streams we can't decode so
long as there is a single valid stream -- through which it might be possible to
create some crazy polyglot attack, a la something like:
https://code.google.com/p/corkami/downloads/detail?name=CorkaMIX.zip

Which is why I suggested previously that attempting to decode the file probably
doesn't get you anything besides wasted cycles :)

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:78: 
Spacing?

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:86: do {
Not quite correct, you may need to call this multiple times for a single packet.
AudioFileReader has a nested while/for loop covering this. Failure to do so may
ignore packet data which is actually invalid,

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:107: } while (base::Time::Now() < deadline &&
result_code >= 0);
&& read_ok

[Lei Zhang, 2013-08-07 20:56:21]

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:645:
PathService::Get(content::DIR_MEDIA_LIBS, &media_path);
Why not just do this in PreSandboxStartup() ?

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:194: // |milliseconds_of_decoding|. It
is still not safe to decode the file in the
Is the time wall clock time, or time in the media file?

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:196:
IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_CheckMediaFile,
Put this in the OS_WIN / OS_MACOSX section above? Ditto elsewhere.

[vandebo (ex-Chrome), 2013-08-07 21:00:43]

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:645:
PathService::Get(content::DIR_MEDIA_LIBS, &media_path);
On 2013/08/07 20:56:21, Lei Zhang wrote:
> Why not just do this in PreSandboxStartup() ?

Layering violation - utility doesn't depend on content

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:194: // |milliseconds_of_decoding|. It
is still not safe to decode the file in the
On 2013/08/07 20:56:21, Lei Zhang wrote:
> Is the time wall clock time, or time in the media file?

"wall clock time" added

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:196:
IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_CheckMediaFile,
On 2013/08/07 20:56:21, Lei Zhang wrote:
> Put this in the OS_WIN / OS_MACOSX section above? Ditto elsewhere.

Also applies to Linux.

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:21: void ClearBoolCallback(bool* called) {
On 2013/08/07 18:27:07, DaleCurtis wrote:
> Name is pretty meaningless.  How about OnError() instead.  Also make it
static.

Done.

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:45: bool MediaFileChecker::Start(const
base::TimeDelta& check_time) {
On 2013/08/07 18:27:07, DaleCurtis wrote:
> Sanity check check_time to be positive and less than some maximum?

Done.

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:50: base::Unretained(&read_ok)));
On 2013/08/07 18:27:07, DaleCurtis wrote:
> Is unretained necessary here? 

Done.

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:63: for (size_t i = 0; i <
format_context->nb_streams; ++i) {
On 2013/08/07 18:27:07, DaleCurtis wrote:
> Just FYI, the way you're doing this doesn't verify that the file is what it
says
> it is, just that we can decode it.  Also, it ignores streams we can't decode
so
> long as there is a single valid stream -- through which it might be possible
to
> create some crazy polyglot attack, a la something like:
> https://code.google.com/p/corkami/downloads/detail?name=CorkaMIX.zip
> 
> Which is why I suggested previously that attempting to decode the file
probably
> doesn't get you anything besides wasted cycles :)

Not what it says it is in what sense?  It doesn't check that the file extension
is correct, right.  It does check that it is a decodable media file.

My thinking on ignoring other streams is that in videos there may be audio
streams in formats that we don't support.

Agreed, this doesn't prevent polyglot attacks, but if it doesn't cause the
decoder to barf, it does require an attacker to try harder.  The code that uses
this sets check_time to 250ms. I agree that it doesn't provide a lot more
assurance, but it does raise the bar somewhat.

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:78: 
On 2013/08/07 18:27:07, DaleCurtis wrote:
> Spacing?

Done.  Don't know how that got in there... :-/

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:86: do {
On 2013/08/07 18:27:07, DaleCurtis wrote:
> Not quite correct, you may need to call this multiple times for a single
packet.
> AudioFileReader has a nested while/for loop covering this. Failure to do so
may
> ignore packet data which is actually invalid,

I don't really understand the API, but I've copied what's in AudioFileReader.

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:107: } while (base::Time::Now() < deadline &&
result_code >= 0);
On 2013/08/07 18:27:07, DaleCurtis wrote:
> && read_ok

Done.

[Lei Zhang, 2013-08-07 22:30:14]

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:645:
PathService::Get(content::DIR_MEDIA_LIBS, &media_path);
On 2013/08/07 21:00:43, vandebo wrote:
> On 2013/08/07 20:56:21, Lei Zhang wrote:
> > Why not just do this in PreSandboxStartup() ?
> 
> Layering violation - utility doesn't depend on content

chrome/DEPS says anyone can use content/public/common.

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:196:
IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_CheckMediaFile,
On 2013/08/07 21:00:43, vandebo wrote:
> On 2013/08/07 20:56:21, Lei Zhang wrote:
> > Put this in the OS_WIN / OS_MACOSX section above? Ditto elsewhere.
> 
> Also applies to Linux.

Oh right. Should this be !(Android || iOS) then, since there's no media
galleries on those platforms?

https://codereview.chromium.org/20572004/diff/36001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/20572004/diff/36001/chrome/utility/DEPS#newcode4
chrome/utility/DEPS:4: "+webkit/glue",
If you don't mind, remove this too.

[DaleCurtis, 2013-08-07 22:56:20]

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
media/base/media_file_checker.cc:86: do {
On 2013/08/07 21:00:43, vandebo wrote:
> On 2013/08/07 18:27:07, DaleCurtis wrote:
> > Not quite correct, you may need to call this multiple times for a single
> packet.
> > AudioFileReader has a nested while/for loop covering this. Failure to do so
> may
> > ignore packet data which is actually invalid,
> 
> I don't really understand the API, but I've copied what's in AudioFileReader.

This is not what's in AudioFileReader, note that AudioFileReader has a
do{}while(packet_size >0) nested inside of a while(av_read_frame()) loop.

[vandebo (ex-Chrome), 2013-08-07 23:09:46]

On 2013/08/07 22:56:20, DaleCurtis wrote:
>
https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
> File media/base/media_file_checker.cc (right):
> 
>
https://codereview.chromium.org/20572004/diff/24001/media/base/media_file_che...
> media/base/media_file_checker.cc:86: do {
> On 2013/08/07 21:00:43, vandebo wrote:
> > On 2013/08/07 18:27:07, DaleCurtis wrote:
> > > Not quite correct, you may need to call this multiple times for a single
> > packet.
> > > AudioFileReader has a nested while/for loop covering this. Failure to do
so
> > may
> > > ignore packet data which is actually invalid,
> > 
> > I don't really understand the API, but I've copied what's in
AudioFileReader.
> 
> This is not what's in AudioFileReader, note that AudioFileReader has a
> do{}while(packet_size >0) nested inside of a while(av_read_frame()) loop.

Sorry, the docs for av_* are scattered and hard to understand.  Can you give me
a hint as to the essential piece I'm missing?  Is it that audio stream data can
span frames, which will always be back to back?  The ac_free_packet for
non-video streams suggested to me that packet was clobbered from frame to
frame...

[vandebo (ex-Chrome), 2013-08-07 23:26:12]

+avi for DEPS changes

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:645:
PathService::Get(content::DIR_MEDIA_LIBS, &media_path);
On 2013/08/07 22:30:14, Lei Zhang wrote:
> On 2013/08/07 21:00:43, vandebo wrote:
> > On 2013/08/07 20:56:21, Lei Zhang wrote:
> > > Why not just do this in PreSandboxStartup() ?
> > 
> > Layering violation - utility doesn't depend on content
> 
> chrome/DEPS says anyone can use content/public/common.

Hmm, indeed.  Fixed.

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/20572004/diff/24001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:196:
IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_CheckMediaFile,
On 2013/08/07 22:30:14, Lei Zhang wrote:
> On 2013/08/07 21:00:43, vandebo wrote:
> > On 2013/08/07 20:56:21, Lei Zhang wrote:
> > > Put this in the OS_WIN / OS_MACOSX section above? Ditto elsewhere.
> > 
> > Also applies to Linux.
> 
> Oh right. Should this be !(Android || iOS) then, since there's no media
> galleries on those platforms?

Done.

https://codereview.chromium.org/20572004/diff/36001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/20572004/diff/36001/chrome/utility/DEPS#newcode4
chrome/utility/DEPS:4: "+webkit/glue",
On 2013/08/07 22:30:14, Lei Zhang wrote:
> If you don't mind, remove this too.

Done.

[Lei Zhang, 2013-08-07 23:32:57]

chrome/ lgtm with some nits:

https://codereview.chromium.org/20572004/diff/33002/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/20572004/diff/33002/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:8: #include "base/files/file_path.h"
Undo

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/DEPS#newcode3
chrome/utility/DEPS:3: "+content/public/common",
Since this is already in chrome/DEPS, you don't need to add this.

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.h:85: void OnCheckMediaFile(int64
milliseconds_of_decoding,
!defined(OS_ANDROID) && !defined(OS_IOS)

[DaleCurtis, 2013-08-08 00:36:07]

Pretty much what I just said, you're only calling decode once per packet when
you need to call it until the packet is exhausted. See the loops I mention here:

Outer loop to read packets:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi...

Inner loop to decode packets:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi...

[vandebo (ex-Chrome), 2013-08-08 05:18:57]

On 2013/08/08 00:36:07, DaleCurtis wrote:
> Pretty much what I just said, you're only calling decode once per packet when
> you need to call it until the packet is exhausted. See the loops I mention
here:

avcodec_decode_audio4 is inside a do while loop on line 106 to 115:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/media_f...

[vandebo (ex-Chrome), 2013-08-08 16:01:47]

-avi: nevermind

https://codereview.chromium.org/20572004/diff/33002/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/20572004/diff/33002/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:8: #include "base/files/file_path.h"
On 2013/08/07 23:32:57, Lei Zhang wrote:
> Undo

It is a missing IWYU.

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/DEPS#newcode3
chrome/utility/DEPS:3: "+content/public/common",
On 2013/08/07 23:32:57, Lei Zhang wrote:
> Since this is already in chrome/DEPS, you don't need to add this.

Done.

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/20572004/diff/33002/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.h:85: void OnCheckMediaFile(int64
milliseconds_of_decoding,
On 2013/08/07 23:32:57, Lei Zhang wrote:
> !defined(OS_ANDROID) && !defined(OS_IOS)

Done.

[DaleCurtis, 2013-08-08 21:04:33]

MediaFileChecker needs a unittest.  Check out audio_file_reader for an example.

https://codereview.chromium.org/20572004/diff/21002/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/20572004/diff/21002/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:538: bool check_success = false;
const and move this declaration below to checker.Start() ?

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:21: const int64 kMaxCheckTimeIneconds = 5;
InSeconds

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:70: stream_contexts[i].codec = codec;
Doesn't look like you actually use codec anywhere.  Just make the map <int,
AVCodecContext*> ?  You can get rid of StreamContext then.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:83: base::Time deadline;
const base::Time deadline = base::Time::Now() + base::TimeDelta::FromSeconds(
    std::max(0, std::min(kMaxCheckTimeInSeconds, check_time)));

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:95: 
I think you need an av_dup_packet(&packet) here otherwise the data referred to
by the packet will get corrupted on the next call to decode.

Once you have that you'll also need an av_free_packet() for any packets you skip
decoding of.  See 143-147 here:

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi...

It's possible this isn't necessary anymore, but we do this everywhere else we
process packets.  The original text with this code can be seen on line 680 here:

https://codereview.chromium.org/10447035/diff/14016/media/filters/ffmpeg_demu...

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:96: if (!ContainsKey(stream_contexts,
packet.stream_index))
You end up doing multiple map look ups below.  Instead just do it once and save
off the AVCodecContext pointer.

std::map<int, StreamContext>::const_iterator it = stream_contexts.find(...);
if (it == stream_contexts.end())
  continue;
AVCodecContext* av_context = it->second;

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:110: &frame_decoded, &packet);
temp_packet

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:115: } while (temp_packet.size  > 0);
Extra space.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:124: av_free_packet(&packet);
This needs to be inside the while loop.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:126: if (result == AVERROR_EOF)
Could be rewritten as:
return read_ok && (result == AVERROR_EOF || result >= 0);

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
File media/base/media_file_checker.h (right):

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.h:19: // Open and read |file| to determine if it
appears to be a valid media file.
Sounds like this comment should go with the constructor and a higher level
comment should be here.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.h:24: explicit MediaFileChecker(base::PlatformFile
file);
Should this be const&?

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.h:30: bool Start(const base::TimeDelta&
check_time);
Time structs are just int64s in the end, so it's not necessary to const& -- see
usage pattern in time.h for reference.

[vandebo (ex-Chrome), 2013-08-08 23:04:18]

On 2013/08/08 21:04:33, DaleCurtis wrote:
> MediaFileChecker needs a unittest.  Check out audio_file_reader for an
example.

Done.  Note, the consumer has a unit test as well:
https://codereview.chromium.org/20190002/diff/181001/chrome/browser/media_gal...

https://codereview.chromium.org/20572004/diff/21002/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/20572004/diff/21002/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:538: bool check_success = false;
On 2013/08/08 21:04:33, DaleCurtis wrote:
> const and move this declaration below to checker.Start() ?

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:21: const int64 kMaxCheckTimeIneconds = 5;
On 2013/08/08 21:04:33, DaleCurtis wrote:
> InSeconds

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:70: stream_contexts[i].codec = codec;
On 2013/08/08 21:04:33, DaleCurtis wrote:
> Doesn't look like you actually use codec anywhere.  Just make the map <int,
> AVCodecContext*> ?  You can get rid of StreamContext then.

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:83: base::Time deadline;
On 2013/08/08 21:04:33, DaleCurtis wrote:
> const base::Time deadline = base::Time::Now() + base::TimeDelta::FromSeconds(
>     std::max(0, std::min(kMaxCheckTimeInSeconds, check_time)));

Used std::min (negative check time will have the same result as 0).

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:95: 
On 2013/08/08 21:04:33, DaleCurtis wrote:
> I think you need an av_dup_packet(&packet) here otherwise the data referred to
> by the packet will get corrupted on the next call to decode.
> 
> Once you have that you'll also need an av_free_packet() for any packets you
skip
> decoding of.  See 143-147 here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/audi...
> 
> It's possible this isn't necessary anymore, but we do this everywhere else we
> process packets.  The original text with this code can be seen on line 680
here:
> 
>
https://codereview.chromium.org/10447035/diff/14016/media/filters/ffmpeg_demu...
> 

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:96: if (!ContainsKey(stream_contexts,
packet.stream_index))
On 2013/08/08 21:04:33, DaleCurtis wrote:
> You end up doing multiple map look ups below.  Instead just do it once and
save
> off the AVCodecContext pointer.
> 
> std::map<int, StreamContext>::const_iterator it = stream_contexts.find(...);
> if (it == stream_contexts.end())
>   continue;
> AVCodecContext* av_context = it->second;

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:110: &frame_decoded, &packet);
On 2013/08/08 21:04:33, DaleCurtis wrote:
> temp_packet

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:115: } while (temp_packet.size  > 0);
On 2013/08/08 21:04:33, DaleCurtis wrote:
> Extra space.

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:115: } while (temp_packet.size  > 0);
BTW - This is what made me think I didn't need the inner do-while loop. It also
doesn't do the temp_packet or av_dup_packet thing. But it only decodes one
stream, so...
https://code.google.com/p/chromium/codesearch/#chromium/src/media/test/ffmpeg...

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:124: av_free_packet(&packet);
On 2013/08/08 21:04:33, DaleCurtis wrote:
> This needs to be inside the while loop.

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.cc:126: if (result == AVERROR_EOF)
On 2013/08/08 21:04:33, DaleCurtis wrote:
> Could be rewritten as:
> return read_ok && (result == AVERROR_EOF || result >= 0);

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
File media/base/media_file_checker.h (right):

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.h:19: // Open and read |file| to determine if it
appears to be a valid media file.
On 2013/08/08 21:04:33, DaleCurtis wrote:
> Sounds like this comment should go with the constructor and a higher level
> comment should be here.

Rephrased comment.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.h:24: explicit MediaFileChecker(base::PlatformFile
file);
On 2013/08/08 21:04:33, DaleCurtis wrote:
> Should this be const&?

Done.

https://codereview.chromium.org/20572004/diff/21002/media/base/media_file_che...
media/base/media_file_checker.h:30: bool Start(const base::TimeDelta&
check_time);
On 2013/08/08 21:04:33, DaleCurtis wrote:
> Time structs are just int64s in the end, so it's not necessary to const& --
see
> usage pattern in time.h for reference.

Done.

[DaleCurtis, 2013-08-09 00:58:04]

lgtm % nits.

https://codereview.chromium.org/20572004/diff/67001/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/20572004/diff/67001/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.h:87: IPC::PlatformFileForTransit
media_file);
Looks like this can be const& ? At least other methods below do that w/ non
const-& messages.

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
media/base/media_file_checker.cc:18: namespace {
media codebase prefers static to unnamed namespace.

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
media/base/media_file_checker.cc:57: if (codec && avcodec_open2(c, codec, NULL)
>= 0) {
No {} to keep style w/ rest of file.

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
File media/base/media_file_checker_unittest.cc (right):

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
media/base/media_file_checker_unittest.cc:15: const base::TimeDelta check_time =
base::TimeDelta::FromMilliseconds(100);
Move inside function to avoid potential static initializer.

[vandebo (ex-Chrome), 2013-08-09 04:26:14]

https://codereview.chromium.org/20572004/diff/67001/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/20572004/diff/67001/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.h:87: IPC::PlatformFileForTransit
media_file);
On 2013/08/09 00:58:04, DaleCurtis wrote:
> Looks like this can be const& ? At least other methods below do that w/ non
> const-& messages.

Fixed all three of these.

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
media/base/media_file_checker.cc:18: namespace {
On 2013/08/09 00:58:04, DaleCurtis wrote:
> media codebase prefers static to unnamed namespace.

But static is deprecated in namespace scope... Done.

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
media/base/media_file_checker.cc:57: if (codec && avcodec_open2(c, codec, NULL)
>= 0) {
On 2013/08/09 00:58:04, DaleCurtis wrote:
> No {} to keep style w/ rest of file.

Oops, Done.

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
File media/base/media_file_checker_unittest.cc (right):

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
media/base/media_file_checker_unittest.cc:13: namespace {
Also killed this namespace.

https://codereview.chromium.org/20572004/diff/67001/media/base/media_file_che...
media/base/media_file_checker_unittest.cc:15: const base::TimeDelta check_time =
base::TimeDelta::FromMilliseconds(100);
On 2013/08/09 00:58:04, DaleCurtis wrote:
> Move inside function to avoid potential static initializer.

Done.

[commit-bot: I haz the power, 2013-08-09 04:27:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/20572004/36002

[commit-bot: I haz the power, 2013-08-09 10:26:27]

Change committed as 216653