Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21953003:
Added logging calls to FFmpegDemuxer. (Closed)
|
Created:
7 years, 4 months ago by Ty Overby Modified:
7 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded logging calls to FFmpegDemuxer.
BUG=260005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215982
Patch Set 1 #Patch Set 2 : Fixed something that got lost in the git madness #
Total comments: 28
Patch Set 3 : #
Total comments: 16
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 16
Patch Set 6 : #Patch Set 7 : rewrote boolean flags to use null pointer checks instead #
Total comments: 4
Patch Set 8 : Added sample format name #
Total comments: 6
Patch Set 9 : #
Total comments: 6
Patch Set 10 : #Patch Set 11 : fixes build on GCC hopefully #Patch Set 12 : #Patch Set 13 : got rid of std::to_string #Patch Set 14 : switched to using double instead of string for file size #
Messages
Total messages: 32 (0 generated)
Here's the first attempt at adding more logging calls.
looking good https://codereview.chromium.org/21953003/diff/5001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/21953003/diff/5001/media/base/media_log.cc#ne... media/base/media_log.cc:183: void MediaLog::SetProperty( these should be SetString/Integer/Double/Boolean as overloading method names is frowned upon (e.g., that bug you hit where it picked the wrong type when using const char*) https://codereview.chromium.org/21953003/diff/5001/media/base/media_log.cc#ne... media/base/media_log.cc:212: const char* key, Value* value) { FYI you're not calling this method -- remove? https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... File media/base/video_decoder_config.cc (right): https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:142: std::string VideoDecoderConfig::FormatName() const { fix indenting https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:143: std::string format_name; you don't need this -- instead just return string https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:144: switch(format()){ since this is a VideoFrame::Format, it should be a helper method on VideoFrame static std::string FormatToString(Format format) https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:166: default: we don't like default cases -- it's better to leave them out and let the compiler complain anytime someone changes the enum definition https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:509: std::string codec_name = codec_context->codec_name; if you're putting the logging here, then we should move all of this down to where you're logging found_audio/video_stream below and use the Audio/VideoDecoderCofnigs for example ... we'll log the wrong thing here if there are multiple audio/video tracks where we don't support the first tracks (notice on line 526 where if we don't accept the config we don't set found_audio_stream to true) also some of these are duplicated (e.g., sample rate == samples per second) https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:540: media_log_->SetProperty("ticks_per_frame", I wouldn't bother logging this one https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:544: base::SStringPrintf(&base_time, "%d/%d", you can use StringPrintf() (no leading S) and pass that result directly into the log call on the next line https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:547: media_log_->SetProperty("time_base", base_time); this might not end up being very useful information ... we'll see :) https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:557: media_log_->SetProperty("video_decoder_config", don't bother logging -- it'll duplicates other stuff you're logging https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:559: media_log_->SetProperty("pixel_format", video_config.FormatName()); nit: s/pixel_format/video_format/ https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:621: media_log_->SetProperty("max_duration", max_duration.InMillisecondsF()); nit: I'd use InSecondsF https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:623: media_log_->SetProperty("filesize_in_bytes",(int) filesize_in_bytes); this will need to be converted to a string due to 32-bit limitations (yep, we've seen people play videos larger than 2^31-1 bytes and complain that media-internals reports a negative number...)
Ok, those things are all fixed. I'm returning inside of the switch statement for my numbers and don't have a return outside of it. (this shouldn't be a problem because my switch/case cover all the possible values). Is Clang sufficiently smart enough to know that I will be returning from inside? Should I count on this behavior in other compilers? https://codereview.chromium.org/21953003/diff/5001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/21953003/diff/5001/media/base/media_log.cc#ne... media/base/media_log.cc:183: void MediaLog::SetProperty( On 2013/08/02 21:53:48, scherkus wrote: > these should be SetString/Integer/Double/Boolean as overloading method names is > frowned upon (e.g., that bug you hit where it picked the wrong type when using > const char*) Done. https://codereview.chromium.org/21953003/diff/5001/media/base/media_log.cc#ne... media/base/media_log.cc:212: const char* key, Value* value) { On 2013/08/02 21:53:48, scherkus wrote: > FYI you're not calling this method -- remove? Done. https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... File media/base/video_decoder_config.cc (right): https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:142: std::string VideoDecoderConfig::FormatName() const { On 2013/08/02 21:53:48, scherkus wrote: > fix indenting Done. https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:143: std::string format_name; On 2013/08/02 21:53:48, scherkus wrote: > you don't need this -- instead just return string Done. https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:144: switch(format()){ On 2013/08/02 21:53:48, scherkus wrote: > since this is a VideoFrame::Format, it should be a helper method on VideoFrame > > > static std::string FormatToString(Format format) Done. https://codereview.chromium.org/21953003/diff/5001/media/base/video_decoder_c... media/base/video_decoder_config.cc:166: default: On 2013/08/02 21:53:48, scherkus wrote: > we don't like default cases -- it's better to leave them out and let the > compiler complain anytime someone changes the enum definition Done. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:509: std::string codec_name = codec_context->codec_name; On 2013/08/02 21:53:48, scherkus wrote: > if you're putting the logging here, then we should move all of this down to > where you're logging found_audio/video_stream below and use the > Audio/VideoDecoderCofnigs > > for example ... we'll log the wrong thing here if there are multiple audio/video > tracks where we don't support the first tracks (notice on line 526 where if we > don't accept the config we don't set found_audio_stream to true) > > also some of these are duplicated (e.g., sample rate == samples per second) Done. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:540: media_log_->SetProperty("ticks_per_frame", On 2013/08/02 21:53:48, scherkus wrote: > I wouldn't bother logging this one Done. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:544: base::SStringPrintf(&base_time, "%d/%d", On 2013/08/02 21:53:48, scherkus wrote: > you can use StringPrintf() (no leading S) and pass that result directly into the > log call on the next line Done. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:547: media_log_->SetProperty("time_base", base_time); On 2013/08/02 21:53:48, scherkus wrote: > this might not end up being very useful information ... we'll see :) Ok, well it's easy to add/remove if it proves useful. FPS is defined in terms of this fraction. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:557: media_log_->SetProperty("video_decoder_config", On 2013/08/02 21:53:48, scherkus wrote: > don't bother logging -- it'll duplicates other stuff you're logging Done. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:559: media_log_->SetProperty("pixel_format", video_config.FormatName()); On 2013/08/02 21:53:48, scherkus wrote: > nit: s/pixel_format/video_format/ Done. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:621: media_log_->SetProperty("max_duration", max_duration.InMillisecondsF()); On 2013/08/02 21:53:48, scherkus wrote: > nit: I'd use InSecondsF Done. https://codereview.chromium.org/21953003/diff/5001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:623: media_log_->SetProperty("filesize_in_bytes",(int) filesize_in_bytes); On 2013/08/02 21:53:48, scherkus wrote: > this will need to be converted to a string due to 32-bit limitations > > (yep, we've seen people play videos larger than 2^31-1 bytes and complain that > media-internals reports a negative number...) Oh wow. Done.
getting closer! https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.cc#n... media/base/media_log.cc:183: void MediaLog::SetStringProperty( try to match the .h order of methods when possible in this case these methods should appear after CreateMediaSourceErrorEvent https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.h#ne... media/base/media_log.h:71: void SetStringProperty(const char* key, const std::string& value); I'd add some docs before this group of methods describing what happens here https://codereview.chromium.org/21953003/diff/15001/media/base/media_log_event.h File media/base/media_log_event.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/media_log_even... media/base/media_log_event.h:91: PROPERTY_CHANGE, docs on this https://codereview.chromium.org/21953003/diff/15001/media/base/video_decoder_... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/video_decoder_... media/base/video_decoder_config.h:117: std::string FormatName() const; remove this method definition -- you no longer implement it https://codereview.chromium.org/21953003/diff/15001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/video_frame.h#... media/base/video_frame.h:56: static std::string FormatToString(Format format) { don't inline this function declaration inside the .h instead keep the definition here but declare it in the .cc /* video_frame.h */ static std::string FormatToString(Format format); /* video_frame.cc */ // static std::string VideoFrame::FormatToString(Format format) { // ... } https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:517: media_log_->SetStringProperty("audio_codec_name", sorry ... I mean moving this down to around lines 614-620 so we do all the logging in one place w/ the final streams we've selected https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:542: std::string codec_name = avcodec_get_name(codec_context->codec_id); ditto for all this logging -- move down to lines 612-620
On 2013/08/02 23:04:27, Ty Overby wrote:
> Ok, those things are all fixed.
>
> I'm returning inside of the switch statement for my numbers and don't have a
> return outside of it. (this shouldn't be a problem because my switch/case
cover
> all the possible values). Is Clang sufficiently smart enough to know that I
> will be returning from inside? Should I count on this behavior in other
> compilers?
My memory is foggy ... we'll find out when we try to CQ the patch.
If a compiler complains the typical course of action is to tack on the following
to the bottom:
std::string MyFunc() {
switch (blah) {
// handle all cases
}
NOTREACHED();
return "";
}
https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.cc#n... media/base/media_log.cc:183: void MediaLog::SetStringProperty( On 2013/08/02 23:18:19, scherkus wrote: > try to match the .h order of methods when possible > > in this case these methods should appear after CreateMediaSourceErrorEvent Done. https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/media_log.h#ne... media/base/media_log.h:71: void SetStringProperty(const char* key, const std::string& value); On 2013/08/02 23:18:19, scherkus wrote: > I'd add some docs before this group of methods describing what happens here Done. https://codereview.chromium.org/21953003/diff/15001/media/base/media_log_event.h File media/base/media_log_event.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/media_log_even... media/base/media_log_event.h:91: PROPERTY_CHANGE, On 2013/08/02 23:18:19, scherkus wrote: > docs on this Done. https://codereview.chromium.org/21953003/diff/15001/media/base/video_decoder_... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/video_decoder_... media/base/video_decoder_config.h:117: std::string FormatName() const; On 2013/08/02 23:18:19, scherkus wrote: > remove this method definition -- you no longer implement it Oh wow. Why isn't this a compile time error? Done. https://codereview.chromium.org/21953003/diff/15001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/21953003/diff/15001/media/base/video_frame.h#... media/base/video_frame.h:56: static std::string FormatToString(Format format) { On 2013/08/02 23:18:19, scherkus wrote: > don't inline this function declaration inside the .h > > instead keep the definition here but declare it in the .cc > > /* video_frame.h */ > static std::string FormatToString(Format format); > > /* video_frame.cc */ > // static > std::string VideoFrame::FormatToString(Format format) { > // ... > } Done. https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:517: media_log_->SetStringProperty("audio_codec_name", On 2013/08/02 23:18:19, scherkus wrote: > sorry ... I mean moving this down to around lines 614-620 so we do all the > logging in one place w/ the final streams we've selected Didn't we discus this earlier? If it is down there then if something breaks, we never get any logging info. If it's up here, then we can see *why* something broke if it did break.
https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:517: media_log_->SetStringProperty("audio_codec_name", On 2013/08/03 00:02:23, Ty Overby wrote: > On 2013/08/02 23:18:19, scherkus wrote: > > sorry ... I mean moving this down to around lines 614-620 so we do all the > > logging in one place w/ the final streams we've selected > > Didn't we discus this earlier? If it is down there then if something breaks, we > never get any logging info. If it's up here, then we can see *why* something > broke if it did break. yeah, but that only works if you save each set of properties for every track the way it is now only shows the last audio/video track that we didn't support because we'll keep overwriting the properties (remember, there could be multiple audio/video tracks within a single file) in the situation where we only support a single audio/video track out of a pair, we'll end up displaying both the one we support and the one we don't how about this: - stick to the simple solution of only logging the "found" streams - add a TODO + file a bug saying we should log all streams - get the UI hooked up and everything working - iterate on getting all streams done after we're happy w/ the simple case https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:524: audio_config.samples_per_second()); fix indent https://codereview.chromium.org/21953003/diff/15001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:619: std::to_string(filesize_in_bytes)); nit: use IntToString() from base/strings/string_number_conversions.h https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.cc... media/base/video_frame.cc:62: return "NATIVE_TEXTURE"; try to have this order match the order defined in the .h (YV12A is last) https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.h#... media/base/video_frame.h:50: #if defined(GOOGLE_TV) can you add this to your .cc? if you don't we'll break our Google TV friends :( https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.h#... media/base/video_frame.h:56: static std::string FormatToString(Format format); docs
https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.cc... media/base/video_frame.cc:62: return "NATIVE_TEXTURE"; On 2013/08/03 00:17:18, scherkus (OOO Aug 3-11) wrote: > try to have this order match the order defined in the .h > > (YV12A is last) Done. https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.h#... media/base/video_frame.h:50: #if defined(GOOGLE_TV) On 2013/08/03 00:17:18, scherkus (OOO Aug 3-11) wrote: > can you add this to your .cc? > > if you don't we'll break our Google TV friends :( Done. https://codereview.chromium.org/21953003/diff/25001/media/base/video_frame.h#... media/base/video_frame.h:56: static std::string FormatToString(Format format); On 2013/08/03 00:17:18, scherkus (OOO Aug 3-11) wrote: > docs Done.
https://codereview.chromium.org/21953003/diff/30001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/21953003/diff/30001/media/base/video_frame.cc... media/base/video_frame.cc:46: switch(format){ Space after ) https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:496: bool found_audio_stream = false; You can remove the bool's now that we can just check for video_stream or audio_stream https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:497: AVStream* audio_stream = nullptr; We're still not C++11 enabled, so this should be NULL. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:501: AVStream* video_stream = nullptr; Ditto. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:595: media_log_->SetBooleanProperty("found_audio_stream", true); Else set to false? https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:609: media_log_->SetBooleanProperty("found_video_stream", true); Ditto? https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:632: std::to_string(filesize_in_bytes)); Indent.
https://codereview.chromium.org/21953003/diff/30001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/21953003/diff/30001/media/base/video_frame.cc... media/base/video_frame.cc:46: switch(format){ On 2013/08/05 19:58:39, DaleCurtis wrote: > Space after ) Done. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:496: bool found_audio_stream = false; On 2013/08/05 19:58:39, DaleCurtis wrote: > You can remove the bool's now that we can just check for video_stream or > audio_stream ??? We still use found_audio_stream and found_video_stream in a few places. Should I remove it and replace it with null pointer checks? https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:497: AVStream* audio_stream = nullptr; On 2013/08/05 19:58:39, DaleCurtis wrote: > We're still not C++11 enabled, so this should be NULL. Done. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:501: AVStream* video_stream = nullptr; On 2013/08/05 19:58:39, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:595: media_log_->SetBooleanProperty("found_audio_stream", true); On 2013/08/05 19:58:39, DaleCurtis wrote: > Else set to false? Done. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:595: media_log_->SetBooleanProperty("found_audio_stream", true); On 2013/08/05 19:58:39, DaleCurtis wrote: > Else set to false? Done. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:609: media_log_->SetBooleanProperty("found_video_stream", true); On 2013/08/05 19:58:39, DaleCurtis wrote: > Ditto? Done. https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:632: std::to_string(filesize_in_bytes)); On 2013/08/05 19:58:39, DaleCurtis wrote: > Indent. Done.
https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:496: bool found_audio_stream = false; On 2013/08/05 20:14:59, Ty Overby wrote: > On 2013/08/05 19:58:39, DaleCurtis wrote: > > You can remove the bool's now that we can just check for video_stream or > > audio_stream > > ??? We still use found_audio_stream and found_video_stream in a few places. > > Should I remove it and replace it with null pointer checks? Yeah.
On 2013/08/05 20:19:22, DaleCurtis wrote: > https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... > File media/filters/ffmpeg_demuxer.cc (right): > > https://codereview.chromium.org/21953003/diff/30001/media/filters/ffmpeg_demu... > media/filters/ffmpeg_demuxer.cc:496: bool found_audio_stream = false; > On 2013/08/05 20:14:59, Ty Overby wrote: > > On 2013/08/05 19:58:39, DaleCurtis wrote: > > > You can remove the bool's now that we can just check for video_stream or > > > audio_stream > > > > ??? We still use found_audio_stream and found_video_stream in a few places. > > > > Should I remove it and replace it with null pointer checks? > > Yeah. Done.
https://codereview.chromium.org/21953003/diff/38001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/38001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:590: AVCodecContext* audio_codec = audio_stream->codec; Do you want to include the audio sample format like you do for video? https://codereview.chromium.org/21953003/diff/38001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:608: std::string codec_name = avcodec_get_name(video_codec->codec_id); Shouldn't this be ->codec_name ?
https://codereview.chromium.org/21953003/diff/38001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/38001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:590: AVCodecContext* audio_codec = audio_stream->codec; On 2013/08/05 21:20:41, DaleCurtis wrote: > Do you want to include the audio sample format like you do for video? Done. https://codereview.chromium.org/21953003/diff/38001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:608: std::string codec_name = avcodec_get_name(video_codec->codec_id); On 2013/08/05 21:20:41, DaleCurtis wrote: > Shouldn't this be ->codec_name ? Done.
https://codereview.chromium.org/21953003/diff/43001/media/base/sample_format.cc File media/base/sample_format.cc (right): https://codereview.chromium.org/21953003/diff/43001/media/base/sample_format.... media/base/sample_format.cc:35: return "UNKNOWN_SAMPLE_FORMAT"; Is this the style you want? Not say, "Unknown", "8-bit unsigned", "32-bit float", etc. https://codereview.chromium.org/21953003/diff/43001/media/base/sample_format.... media/base/sample_format.cc:48: case kSampleFormatMax: Just break here and rely on the NOTREACHED() below. https://codereview.chromium.org/21953003/diff/43001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/43001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:631: media_log_->SetBooleanProperty("found_vide_stream", false); Misspelled?
https://codereview.chromium.org/21953003/diff/43001/media/base/sample_format.cc File media/base/sample_format.cc (right): https://codereview.chromium.org/21953003/diff/43001/media/base/sample_format.... media/base/sample_format.cc:35: return "UNKNOWN_SAMPLE_FORMAT"; On 2013/08/05 22:16:54, DaleCurtis wrote: > Is this the style you want? Not say, "Unknown", "8-bit unsigned", "32-bit > float", etc. I like yours more. Done. https://codereview.chromium.org/21953003/diff/43001/media/base/sample_format.... media/base/sample_format.cc:48: case kSampleFormatMax: On 2013/08/05 22:16:54, DaleCurtis wrote: > Just break here and rely on the NOTREACHED() below. Done. https://codereview.chromium.org/21953003/diff/43001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/43001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:631: media_log_->SetBooleanProperty("found_vide_stream", false); On 2013/08/05 22:16:54, DaleCurtis wrote: > Misspelled? Done.
https://codereview.chromium.org/21953003/diff/49001/media/base/sample_format.cc File media/base/sample_format.cc (right): https://codereview.chromium.org/21953003/diff/49001/media/base/sample_format.... media/base/sample_format.cc:35: return "Unknown Sample Format"; Lower case second word+ https://codereview.chromium.org/21953003/diff/49001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/49001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:596: media_log_->SetStringProperty("sample_format", sample_name); audio_sample_format to keep consistency with other names? Alternatively, drop audio_ from all properties if they're going to be under an Audio heading or something. https://codereview.chromium.org/21953003/diff/49001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:637: media_log_->SetStringProperty("filesize_in_bytes", Why is this a string property?
https://codereview.chromium.org/21953003/diff/49001/media/base/sample_format.cc File media/base/sample_format.cc (right): https://codereview.chromium.org/21953003/diff/49001/media/base/sample_format.... media/base/sample_format.cc:35: return "Unknown Sample Format"; On 2013/08/05 22:33:35, DaleCurtis wrote: > Lower case second word+ Done. https://codereview.chromium.org/21953003/diff/49001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/21953003/diff/49001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:596: media_log_->SetStringProperty("sample_format", sample_name); On 2013/08/05 22:33:35, DaleCurtis wrote: > audio_sample_format to keep consistency with other names? Alternatively, drop > audio_ from all properties if they're going to be under an Audio heading or > something. When the new UI goes up they will be, but until then, we can't do anything special. I'll add the "audio_". Done. https://codereview.chromium.org/21953003/diff/49001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:637: media_log_->SetStringProperty("filesize_in_bytes", On 2013/08/05 22:33:35, DaleCurtis wrote: > Why is this a string property? From scherkus@ > this will need to be converted to a string due to 32-bit limitations > (yep, we've seen people play videos larger than 2^31-1 bytes and > complain that > media-internals reports a negative number...)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/21953003/58001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/21953003/70001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/21953003/80001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/21953003/93001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tyoverby@chromium.org/21953003/110001
Message was sent while issue was closed.
Change committed as 215982
Message was sent while issue was closed.
On 2013/08/06 21:30:32, I haz the power (commit-bot) wrote: > Change committed as 215982 Nice CL -- thanks for reviewing while I was OOO. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
