Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1728193004:
Media: Do not support new codecs with legacy MIME type names. (Closed)
|
Created:
4 years, 10 months ago by ddorwin Modified:
4 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia: Do not support new codecs with legacy MIME type names.
Do not advertise support for newer codecs with x-m4a or x-m4v, which we only
support for compatibility reasons. This restores these MIME types to the
behavior before the recent additions of new codecs.
Also, do not advertise newer codecs for HLS unless/until there is a need.
This also replaces the |kFormatCodecMappings| static table, which was used
to build |media_format_map_| with explicit calls for each container MIME type.
This allows the codec lists to be built programatically, providing more
control and less literal duplication than literal constants.
BUG=589675
Committed: https://crrev.com/bc4545b1289c74b44ed6f86c124391d5366eb4e3
Cr-Commit-Position: refs/heads/master@{#384781}
Patch Set 1 #Patch Set 2 : Review obsolete avc1 comments. #
Total comments: 14
Patch Set 3 : review feedback #Patch Set 4 : rebase only #Patch Set 5 : prototype new approach; drop MP3 from most types #
Total comments: 7
Patch Set 6 : use const char for some literals; restore MP3 for HLS #Patch Set 7 : revert to std::string #Patch Set 8 : rebase only #Patch Set 9 : Eliminate kFormatCodecMappings entirely; handle proprietary codecs. #Patch Set 10 : Reorder some types for consistency #
Total comments: 22
Patch Set 11 : fix Android build #Patch Set 12 : Fix CodecSupportTest_Mpeg2Ts on Cast #Patch Set 13 : review feedback #
Total comments: 7
Patch Set 14 : review feedback #Patch Set 15 : fix Cast builds #Patch Set 16 : rebase only #Patch Set 17 : Fix Android - extra comma caused empty string codec #
Messages
Total messages: 47 (21 generated)
ddorwin@chromium.org changed reviewers: + dalecurtis@chromium.org, servolk@chromium.org
ddorwin@chromium.org changed required reviewers: + dalecurtis@chromium.org
WDYT? It uses macros, but in a very limited scope. This also allows us to remove duplicated audio codec strings. Irrespective of this CL, the better solution is to use an array of enums instead of a text list (see the existing comments). I tried that once, but variable-length arrays of codecs was going to make it much more complex. https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... File media/base/mime_util_internal.cc (left): https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:45: // avc1.42E0xx - H.264 Baseline These three lines were not necessary before either since the related code was moved long ago. https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:125: {"application/x-mpegurl", PROPRIETARY, kOriginalMp4VideoCodecsExpression}, servolk: We don't have tests for the newer codecs, but I don't believe we intend to support them on the .src=HLS case.
Kind of dicy with macro usage like this. Could we instead make these additions dynamically during the building of the format map? I.e. leave them out of kFormatCodecMappings and add an extra step in the map building code to insert these using normal string concatenation functionality.
https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:45: "mp4a.66,mp4a.67,mp4a.68,mp4a.69,mp4a.6B,mp4a.40.2,mp4a.40.02,mp4a.40.5," \ Let's clean up those lists a little, while we are at it. Let's remove mp4a.40.02 and mp4a.40.05 since they represent the same codecs as the ids without leading zero in the last element. https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:64: #define ORIGINAL_MP4_VIDEO_CODECS "avc1.42E00A,avc1.4D400A,avc1.64000A" Now that all three of these codec ids map to the same value MimeUtil::H264 we could keep just one of them. https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:81: static const char kOriginalMp4VideoCodecsExpression[] = Each of k*Expression is only used once, so perhaps it would be easier to use those expressions directly in the kFormatCodecMappings definition for saving one level of indirection? Or is there some value in having those declared as variables? https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:97: {"video/webm", COMMON, "opus,vorbis,vp8,vp8.0,vp9,vp9.0"}, vp8.0 and vp9.0 can be removed, those variants are already present in kUnambiguousCodecStringMap. https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:125: {"application/x-mpegurl", PROPRIETARY, kOriginalMp4VideoCodecsExpression}, On 2016/02/25 01:09:47, ddorwin wrote: > servolk: We don't have tests for the newer codecs, but I don't believe we intend > to support them on the .src=HLS case. Acknowledged. Yes, AFAIK neither HEVC nor AC3/EAC3 are support for Android HLS, only for Chromecast's.
On 2016/02/25 01:40:17, DaleCurtis wrote:
> Kind of dicy with macro usage like this. Could we instead make these additions
> dynamically during the building of the format map? I.e. leave them out of
> kFormatCodecMappings and add an extra step in the map building code to insert
> these using normal string concatenation functionality.
Yes, we could do this dynamically in InitializeMimeTypeMaps(). That would also
make it simpler to use the Codec enum values rather strings that we convert to
the enum.
However, I think this would make it much more difficult to read/understand the
list of supported types. (This CL is already making that a bit more abstract.)
Specifically, rather than kFormatCodecMappings, we would have a series of calls
for the MIME types. For example:
AddContainerWithCodecs("audio/wav", "1", COMMON);
...
AddContainerWithMp4Codecs("video/mp4");
We would have a couple options for minimizing duplication as I am trying to do
in this CL.
1. Use parameters (i.e. is_video_container, is_legacy_container)
1. Have separate functions (i.e. AddOriginalMp4VideoCodecs()).
WDYT? Other ideas?
https://codereview.chromium.org/1728193004/diff/20001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):
https://codereview.chromium.org/1728193004/diff/20001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1314:
CanPlay("'application/vnd.apple.mpegurl; codecs=\"hev1.1.6.L93.B0\"'"));
This test is correct but would probably fail in certain configurations today.
Same for the x-mpegurl tests above.
https://codereview.chromium.org/1728193004/diff/20001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1324: EXPECT_EQ(kNot,
CanPlay("'application/vnd.apple.mpegurl; codecs=\"ac-3\"'"));
ditto
https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in...
File media/base/mime_util_internal.cc (right):
https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in...
media/base/mime_util_internal.cc:45:
"mp4a.66,mp4a.67,mp4a.68,mp4a.69,mp4a.6B,mp4a.40.2,mp4a.40.02,mp4a.40.5," \
On 2016/02/25 01:46:06, servolk wrote:
> Let's clean up those lists a little, while we are at it. Let's remove
> mp4a.40.02 and mp4a.40.05 since they represent the same codecs as the ids
> without leading zero in the last element.
Let's handle that in
https://bugs.chromium.org/p/chromium/issues/detail?id=591869. Added TODO.
https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in...
media/base/mime_util_internal.cc:64: #define ORIGINAL_MP4_VIDEO_CODECS
"avc1.42E00A,avc1.4D400A,avc1.64000A"
On 2016/02/25 01:46:06, servolk wrote:
> Now that all three of these codec ids map to the same value MimeUtil::H264 we
> could keep just one of them.
Done.
https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in...
media/base/mime_util_internal.cc:81: static const char
kOriginalMp4VideoCodecsExpression[] =
On 2016/02/25 01:46:06, servolk wrote:
> Each of k*Expression is only used once, so perhaps it would be easier to use
> those expressions directly in the kFormatCodecMappings definition for saving
one
> level of indirection? Or is there some value in having those declared as
> variables?
These are shorter. Also, this allows us to minimize the scope of the macros.
However, this may be moot if we change the approach.
https://codereview.chromium.org/1728193004/diff/20001/media/base/mime_util_in...
media/base/mime_util_internal.cc:97: {"video/webm", COMMON,
"opus,vorbis,vp8,vp8.0,vp9,vp9.0"},
On 2016/02/25 01:46:06, servolk wrote:
> vp8.0 and vp9.0 can be removed, those variants are already present in
> kUnambiguousCodecStringMap.
Done.
I don't really follow; maybe my suggestion wasn't clear. I'm essentially just
advocating something like:
std::string kOriginalCodecs = "..."
std::string kNewCodecs = "..."
std::string kOriginalMp4AUdioCodecs = "..."
AddContainerWithCodecs("audio/x-m4a", PROPRIETARY, kOriginalMp4AudioCodecs);
It's essentially exactly what you're already doing without the #define magic.
That said original vs new is also a bit of a misnomer. What does new mean next
year? :)
https://codereview.chromium.org/1728193004/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1728193004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:1314: CanPlay("'application/vnd.apple.mpegurl; codecs=\"hev1.1.6.L93.B0\"'")); On 2016/03/04 01:26:29, ddorwin wrote: > This test is correct but would probably fail in certain configurations today. > Same for the x-mpegurl tests above. It shouldn't fail in any of the currently supported configurations. The only way to get outcome other than kNot here is to have both HLS and HEVC support enabled. Chromecast is the only platform that enables HEVC, but it doesn't support HLS mime types being used for this test (Chromecast uses video/mp2t mime type for HLS). Android is the only platform that supports these HLS mime types, but it currently doesn't enable HEVC.
On 2016/03/04 01:39:02, DaleCurtis wrote:
> I don't really follow; maybe my suggestion wasn't clear. I'm essentially just
> advocating something like:
> std::string kOriginalCodecs = "..."
> std::string kNewCodecs = "..."
> std::string kOriginalMp4AUdioCodecs = "..."
>
> AddContainerWithCodecs("audio/x-m4a", PROPRIETARY, kOriginalMp4AudioCodecs);
>
> It's essentially exactly what you're already doing without the #define magic.
Okay, that's similar to what I was proposing except better in that all the
literals are together. I'll try that.
> That said original vs new is also a bit of a misnomer. What does new mean next
> year? :)
Original is the original set of MP4 codecs that were used with the legacy types.
New is everything else. Next year, everything else is added to new. I'll try to
improve the naming, though.
Patchset #4 (id:60001) has been deleted
I prototyped the new approach. It is incomplete, but it passes the MediaCanPlayTypeTests. Please provide comments before I make further changes. This is also rebased (PS4) on other recent changes, so it may be best do diff against base. https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:59: // Most MPEG MIME types are added in InitializeMimeTypeMaps(). We now have this information scattered. Should we move this entire table to InitializeMimeTypeMaps? it seems better to have them together. Also, this would allow us to clean up the OS_ANDROID code above in a similar way. https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:316: // kFormatCodecMappings with PROPRIETARY. I will add this parameter to AddContainerWithCodecs() and use it to build a vector of proprietary MIME types to be removed (rather than iterating through kFormatCodecMappings to identify them). https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:317: AddContainerWithCodecs("audio/mp4", mp4_audio_codecs); Each entry remaining in kFormatCodecMappings would look something like this, but with a literal. Those should be a lot cleaner than this. Still, the strings above will be in the middle of such entries unless we want multiple #ifdef's.
https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:295: #if defined(USE_PROPRIETARY_CODECS) Could have an InitializeProprietaryCodecMimeTypeMaps() here. https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:298: const std::string aac = "mp4a.66,mp4a.40.2"; // MPEG-2 AAC and MPEG-4 AAC. You can keep these as const char* up above and just initialize the std::string versions for globing here; that might make the readability better.
Addressed one of your comments and replied to both. Do you have thoughts on eliminating kFormatCodecMappings entirely? https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:295: #if defined(USE_PROPRIETARY_CODECS) On 2016/03/17 21:00:21, DaleCurtis wrote: > Could have an InitializeProprietaryCodecMimeTypeMaps() here. Yes, though that would separate the lists of types even further, and it may not always be appropriate (i.e. https://crbug.com/327115). https://codereview.chromium.org/1728193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:298: const std::string aac = "mp4a.66,mp4a.40.2"; // MPEG-2 AAC and MPEG-4 AAC. On 2016/03/17 21:00:21, DaleCurtis wrote: > You can keep these as const char* up above and just initialize the std::string > versions for globing here; that might make the readability better. I've uploaded this change, but I plan to revert - I think this is worse because it further separates those literals from others and I have to add std::string() in places. This is really a minor issue, though, because https://crbug.com/461009 should eliminate the need for theses.
I'm fine with eliminating the static map.
Description was changed from ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also eliminates some literal duplication between audio and video constants. BUG=589675 ========== to ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also eliminates some literal duplication between audio and video constants. BUG=589675 ==========
ddorwin@chromium.org changed reviewers: + hubbe@chromium.org
Description was changed from ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also eliminates some literal duplication between audio and video constants. BUG=589675 ========== to ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also replaces the |kFormatCodecMappings| static table, which was used to build |media_format_map_| with explicit calls for each container MIME type. This allows the codec lists to be built programatically, providing more control and less literal duplication than literal constants. BUG=589675 ==========
This is now ready for review.
Nice cleanup! https://codereview.chromium.org/1728193004/diff/200001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1728193004/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:197: // Remove all but "audio/mpeg" when https://crbug.com/592889 is fixed. Why this style here instead of the EXPECT syntax like used elsewhere? https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:234: void MimeUtil::InitializeMimeTypeMaps() { Out of curiosity how long does this function take to execute? https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:252: // Each call to AddContainerWithCodecs() contains a media type Typically we put these comments in the .h file, but I don't have a strong preference. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:262: #if !defined(OS_ANDROID) Kind of ugly, I'd prefer to just have "const std::string ogg_video_codecs;" in the else clause. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:264: #endif Lots of defs in this area so please include the // #DEFINE at each #endif. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:275: #if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING) Similarly these are a little error prone, might be easier to copy/paste the name and fully define each variable within each #if clause. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:280: #if BUILDFLAG(ENABLE_HEVC_DEMUXING) Ditto. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:287: AddContainerWithCodecs("audio/wav", "1", false); As an aside, what's the "1" about? https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:295: AddContainerWithCodecs("video/ogg", ogg_codecs, false); ogg_video_codecs? https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:340: for (size_t j = 0; j < mime_type_codecs.size(); ++j) { for (const auto& mime_codec : mime_type_codecs) ?
Patchset #12 (id:240001) has been deleted
https://codereview.chromium.org/1728193004/diff/200001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1728193004/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:197: // Remove all but "audio/mpeg" when https://crbug.com/592889 is fixed. On 2016/03/31 22:19:46, DaleCurtis wrote: > Why this style here instead of the EXPECT syntax like used elsewhere? This test is run for all MPEG container types. All but one of them _should_ support "mp3" as a string, but a few do because of the bug above. This CL allows us to exclude most of them (because the containers don't support other MP3 strings), so we can expect kNot here and remove the individual tests (see removed lines in base). These types do incorrectly allow "mp3", so we must not run this expectation for them. They also have individual tests referencing this bug. Even if/when we fix this bug, we would need this condition for "audio/mpeg" unless we want to duplicate this everywhere. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:234: void MimeUtil::InitializeMimeTypeMaps() { On 2016/03/31 22:19:46, DaleCurtis wrote: > Out of curiosity how long does this function take to execute? I'll find out. The std::string operations are new. The many calls to AddContainerWithCodecs replace iteration, though that didn't make a function call. We might get a little bit back when fixing https://crbug.com/461009 eliminates the use of strings and the enum lookup. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:252: // Each call to AddContainerWithCodecs() contains a media type On 2016/03/31 22:19:46, DaleCurtis wrote: > Typically we put these comments in the .h file, but I don't have a strong > preference. This seems to be implementation details. Note that the function name is *not* this function. The next paragraph is about the literals used. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:262: #if !defined(OS_ANDROID) On 2016/03/31 22:19:46, DaleCurtis wrote: > Kind of ugly, I'd prefer to just have "const std::string ogg_video_codecs;" in > the else clause. I changed it to add theora since there is more common code, but I can switch to this if you prefer. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:264: #endif On 2016/03/31 22:19:46, DaleCurtis wrote: > Lots of defs in this area so please include the // #DEFINE at each #endif. Done. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:275: #if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING) On 2016/03/31 22:19:46, DaleCurtis wrote: > Similarly these are a little error prone, might be easier to copy/paste the name > and fully define each variable within each #if clause. In this case, it's not just the variable name - we would need to duplicate part of the definition. In this case, line 278. I think such duplication would be more error prone than this. Another option would be to define ac3_and_eac3, which would be the empty string when this is not defined, and add those to mp4_audio_codecs. That sort of implies that those types are supported, so that seems a bit worse. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:280: #if BUILDFLAG(ENABLE_HEVC_DEMUXING) On 2016/03/31 22:19:46, DaleCurtis wrote: > Ditto. See above. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:287: AddContainerWithCodecs("audio/wav", "1", false); On 2016/03/31 22:19:46, DaleCurtis wrote: > As an aside, what's the "1" about? I don't know. It's always been that way. There is a comment at line 33, but it is not very helpful. https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:295: AddContainerWithCodecs("video/ogg", ogg_codecs, false); On 2016/03/31 22:19:46, DaleCurtis wrote: > ogg_video_codecs? Naming fun. "video/foo" supports all foo codecs. This is consistent with mp4_codecs below. I agree that it is a bit confusing when it appears with "video/..." Any suggestions? https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:340: for (size_t j = 0; j < mime_type_codecs.size(); ++j) { On 2016/03/31 22:19:46, DaleCurtis wrote: > for (const auto& mime_codec : mime_type_codecs) ? Done. This was just moved. :) https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... media/base/mime_util_internal.cc:276: #endif // BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING) git cl format did this.
lgtm % nits. https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... media/base/mime_util_internal.cc:273: const std::string mp4_audio_codecs = Can't you do what you did for theora? std::string mp4_audio_codecs = aac + "," + mp3; #if ... mp4_audio_codecs += "ac-3,ec-3," .. #endif https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... media/base/mime_util_internal.cc:278: const std::string mp4_video_codecs = Ditto. https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... media/base/mime_util_internal.cc:429: for (const std::string& container : proprietary_media_containers_) Use auto here too or std::string above.
https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:234: void MimeUtil::InitializeMimeTypeMaps() { On 2016/03/31 23:24:57, ddorwin wrote: > On 2016/03/31 22:19:46, DaleCurtis wrote: > > Out of curiosity how long does this function take to execute? > > I'll find out. > > The std::string operations are new. The many calls to AddContainerWithCodecs > replace iteration, though that didn't make a function call. > > We might get a little bit back when fixing https://crbug.com/461009 eliminates > the use of strings and the enum lookup. For a Linux x64 release build on a workstation: 45 to 145 microseconds. https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... media/base/mime_util_internal.cc:273: const std::string mp4_audio_codecs = On 2016/03/31 23:31:34, DaleCurtis wrote: > Can't you do what you did for theora? > > std::string mp4_audio_codecs = aac + "," + mp3; > #if ... > mp4_audio_codecs += "ac-3,ec-3," .. > #endif Done. Thanks. https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... media/base/mime_util_internal.cc:278: const std::string mp4_video_codecs = On 2016/03/31 23:31:34, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/1728193004/diff/280001/media/base/mime_util_i... media/base/mime_util_internal.cc:429: for (const std::string& container : proprietary_media_containers_) On 2016/03/31 23:31:33, DaleCurtis wrote: > Use auto here too or std::string above. Done.
The CQ bit was checked by ddorwin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1728193004/#ps320001 (title: "fix Cast builds")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1728193004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1728193004/320001
The CQ bit was unchecked by ddorwin@chromium.org
The CQ bit was checked by ddorwin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1728193004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1728193004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by ddorwin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1728193004/#ps340001 (title: "rebase only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1728193004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1728193004/340001
https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1728193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:234: void MimeUtil::InitializeMimeTypeMaps() { On 2016/04/01 00:01:41, ddorwin wrote: > On 2016/03/31 23:24:57, ddorwin wrote: > > On 2016/03/31 22:19:46, DaleCurtis wrote: > > > Out of curiosity how long does this function take to execute? > > > > I'll find out. > > > > The std::string operations are new. The many calls to AddContainerWithCodecs > > replace iteration, though that didn't make a function call. > > > > We might get a little bit back when fixing https://crbug.com/461009 eliminates > > the use of strings and the enum lookup. > > For a Linux x64 release build on a workstation: 45 to 145 microseconds. For the record, without this CL, the equivalent code (starting with "// Initialize the supported media formats.") took 45 to 135 microseconds.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ddorwin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1728193004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1728193004/340001
Patchset #17 (id:360001) has been deleted
The CQ bit was checked by ddorwin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1728193004/#ps380001 (title: "Fix Android - extra comma caused empty string codec")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1728193004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1728193004/380001
Message was sent while issue was closed.
Description was changed from ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also replaces the |kFormatCodecMappings| static table, which was used to build |media_format_map_| with explicit calls for each container MIME type. This allows the codec lists to be built programatically, providing more control and less literal duplication than literal constants. BUG=589675 ========== to ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also replaces the |kFormatCodecMappings| static table, which was used to build |media_format_map_| with explicit calls for each container MIME type. This allows the codec lists to be built programatically, providing more control and less literal duplication than literal constants. BUG=589675 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also replaces the |kFormatCodecMappings| static table, which was used to build |media_format_map_| with explicit calls for each container MIME type. This allows the codec lists to be built programatically, providing more control and less literal duplication than literal constants. BUG=589675 ========== to ========== Media: Do not support new codecs with legacy MIME type names. Do not advertise support for newer codecs with x-m4a or x-m4v, which we only support for compatibility reasons. This restores these MIME types to the behavior before the recent additions of new codecs. Also, do not advertise newer codecs for HLS unless/until there is a need. This also replaces the |kFormatCodecMappings| static table, which was used to build |media_format_map_| with explicit calls for each container MIME type. This allows the codec lists to be built programatically, providing more control and less literal duplication than literal constants. BUG=589675 Committed: https://crrev.com/bc4545b1289c74b44ed6f86c124391d5366eb4e3 Cr-Commit-Position: refs/heads/master@{#384781} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/bc4545b1289c74b44ed6f86c124391d5366eb4e3 Cr-Commit-Position: refs/heads/master@{#384781} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
