Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(115)

Issue 2347863003: H264 codec: Check profile-level-id when matching (Closed)

Created:
4 years, 3 months ago by magjed_webrtc
Modified:
4 years, 1 month ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

H264 codec: Check profile-level-id when matching For the H264 video codec, comparing the codec name is not enough for determining a match. The profile-level-id must also match. This CL: - Specializes the VideoCodec::Matches function with extra logic for matching H264 codecs. - Adds unittests for matching H264 video codecs. - Removes the unnecessary CodecTest fixture class. BUG=webrtc:6337, chromium:645599 Committed: https://crrev.com/68979ab7dd971ab6e983b23c83154ba05e183fb8 Cr-Commit-Position: refs/heads/master@{#14546}

Patch Set 1 #

Total comments: 13

Patch Set 2 : Fix hta comments #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -17 lines) Patch
M webrtc/media/base/codec.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/media/base/codec.cc View 1 2 chunks +38 lines, -0 lines 9 comments Download
M webrtc/media/base/codec_unittest.cc View 1 12 chunks +77 lines, -17 lines 0 comments Download
M webrtc/media/base/mediaconstants.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/media/base/mediaconstants.cc View 1 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 30 (15 generated)
magjed_webrtc
Please take a look.
4 years, 3 months ago (2016-09-18 15:04:23 UTC) #9
hta-webrtc
Some commentary. Profile-level-id is tricky. Also some thoughts on structuring. https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.cc File webrtc/media/base/codec.cc (right): https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.cc#newcode257 ...
4 years, 3 months ago (2016-09-19 06:38:36 UTC) #10
magjed_webrtc
https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.cc File webrtc/media/base/codec.cc (right): https://codereview.webrtc.org/2347863003/diff/40001/webrtc/media/base/codec.cc#newcode257 webrtc/media/base/codec.cc:257: if (name != kH264CodecName) On 2016/09/19 06:38:36, hta-webrtc wrote: ...
4 years, 3 months ago (2016-09-19 12:40:31 UTC) #11
kthelgason
I'm not qualified to comment on whether all the logic regarding comparison of codecs, but ...
4 years, 3 months ago (2016-09-20 13:34:13 UTC) #13
magjed_webrtc
Ping Harald, can you take another look? This CL is a first step towards proper ...
4 years, 2 months ago (2016-09-30 12:33:32 UTC) #14
hta-webrtc
Lgtm Apologies for the delay. I still think it's worth it setting the bits in ...
4 years, 2 months ago (2016-10-05 11:56:29 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2347863003/60001
4 years, 2 months ago (2016-10-05 14:33:20 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on ...
4 years, 2 months ago (2016-10-05 16:33:51 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2347863003/60001
4 years, 2 months ago (2016-10-06 09:07:56 UTC) #21
commit-bot: I haz the power
Committed patchset #2 (id:60001)
4 years, 2 months ago (2016-10-06 09:15:52 UTC) #23
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/68979ab7dd971ab6e983b23c83154ba05e183fb8 Cr-Commit-Position: refs/heads/master@{#14546}
4 years, 2 months ago (2016-10-06 09:16:00 UTC) #25
magjed_webrtc
A revert of this CL (patchset #2 id:60001) has been created in https://codereview.webrtc.org/2440123002/ by magjed@webrtc.org. ...
4 years, 2 months ago (2016-10-21 14:08:48 UTC) #26
tommi
since this will be relanded, I figured I'd share some suggestions for smaller, faster code ...
4 years, 1 month ago (2016-10-25 09:19:58 UTC) #28
hta-webrtc
A few words of explanation to Tommi's comment. https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.cc File webrtc/media/base/codec.cc (right): https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.cc#newcode282 webrtc/media/base/codec.cc:282: their_profile_level_id.substr(0, ...
4 years, 1 month ago (2016-10-25 09:36:26 UTC) #29
magjed_webrtc
4 years, 1 month ago (2016-10-25 10:58:01 UTC) #30
Message was sent while issue was closed.
https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:29: return (iter == codec.params.end()) ?
default_value : iter->second;
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> nit:
> 
> this function should just return |const char*|.  The values for key and
> default_value are always const char*, so there's no need for code that copies
> them or to create a string copy for return. |codec| is also passed in, so
> returning iter->second.c_str() should be fine.

The reason for not returning char* was that CodecParameterMap is holding
strings. I can do iter->second.c_str() like you say, but that's a raw pointer
and we need to be careful to sync the lifetime with |codec|, but yes it's
possible.

I can make the key arg char* as well. I don't think it will make a difference
performance-wise since std::map<std::string, std::string>::find will convert it
to a std::string.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:279: GetParamOrDefault(codec,
kH264FmtpLevelAsymmetryAllowed, "0") == "1";
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> nit: "0" -> "" (or nullptr?)

Yes, should be possible. I wanted to be clear here. "0" is the default value for
level asymmetry if it's not set explicitly.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:282: their_profile_level_id.substr(0, 4));
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> three nits:
> 
> * do you know if our_profile_level_id.length() >= 4? (same for
> their_profile_level_id).
> * it would be good to document why we use 4, or have a constant for it.
> * There's no need to create two string copies just to compare sub strings. 
> Instead you could use strncmp or string::compare().
> 

I will add explicit functionality for parsing the profile-level-id string into
two enums (profile (BP, CBP, MP, HP) + level) in a separate CL, so these
comments will be automatically addressed there.

https://codereview.webrtc.org/2347863003/diff/60001/webrtc/media/base/codec.c...
webrtc/media/base/codec.cc:283: return level_asymmetry_allowed &&
is_profile_match;
On 2016/10/25 09:19:58, tommi (webrtc) wrote:
> nit: if level_asymmetry_allowed is required, you might as well check that and
> return before checking for a profile match.

Sure. As long as the code is as clear as possible. This is not a tight function.

Powered by Google App Engine
This is Rietveld 408576698