Closed Bug 1407919 Opened 7 years ago Closed 7 years ago

Supports new RFC-6381 VP9 codec string

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

Product:
Core
Component:
Audio/Video: Playback
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

(
URL
)

Details

Attachments

(7 files)

Bug 1407919 - Part1 - Add an extraction function to parse the RFC-6381 VP9 codec string.
59 bytes, text/x-review-board-request
jya
: review+
mozbugz
: review+
Details
Bug 1407919 - Part2 - Use the helper function to tell if it is a vp8/9 codec string.
59 bytes, text/x-review-board-request
Details
Bug 1407919 - Part3 - Add a gtest for testing the extraction function.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1407919 - Part4 - Add mBitDepth field into VideoInfo.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1407919 - Part7 - Check bit depth in PDM::Supports.
59 bytes, text/x-review-board-request
mozbugz
: review+
jya
: review+
Details
Bug 1407919 - Part5 - Extract bit depth information from codec parameter string into VideoInfo::mBitDepth.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1407919 - Part6 - Check bit depth in WebMDecoder to determine if we support HDR.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Some sites like netflix will use MediaSource.isTypeSupported('video/mp4;codecs=vp09.00.11.08.02') to ask if we are supported this mimetype with codecs. Currently, we only accept "video/mp4;codecs=vp9". The codec string is defined in the spec: https://www.webmproject.org/vp9/mp4/#codecs-parameter-string I can simply modified the code from [1] to if (codec.EqualsLiteral("vp9") || codec.EqualsLiteral("vp9.0") || (NS_ConvertUTF16toUTF8(codec).Find("vp09", true) != -1)) { trackInfos.AppendElement( to make MediaSource.isTypeSupported('video/mp4;codecs=vp09.00.11.08.02') works... But we should handle it comprehensively in other XXXDecoder::IsSupportedType. [1] http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/dom/media/fmp4/MP4Decoder.cpp#113
string is following the format: <sample entry 4CC>.<profile>.<level>.<bitDepth>.<chromaSubsampling>. <colourPrimaries>.<transferCharacteristics>.<matrixCoefficients>. <videoFullRangeFlag> (this could be use for h264 too)
URL: https://www.webmproject.org/vp9/mp4/#...
Summary: Accept mimetype with codecs parameter string starting with vp09 → Supports new RFC-6381 VP9 codec string
Assignee: nobody → jacheng
Blocks: 1408302
Comment on attachment 8918167 [details] Bug 1407919 - Part1 - Add an extraction function to parse the RFC-6381 VP9 codec string. https://reviewboard.mozilla.org/r/189028/#review194390 this must be changed to use the mozilla coding style. ::: dom/media/VideoUtils.h:246 (Diff revision 2) > bool > ExtractH264CodecDetails(const nsAString& aCodecs, > int16_t& aProfile, > int16_t& aLevel); > > +struct VideoColorSpace { { on the next line. Please make sure you pass your code to clang-format in mozilla style first. (with very recent clang-format ./mach clang-format will do it for you ::: dom/media/VideoUtils.cpp:275 (Diff revision 2) > + } > + // Start to validate the parsing value. > + > + // profile should be 0,1,2 or 3. > + // See https://www.webmproject.org/vp9/profiles/ > + if (aProfile > 3) { we don't support more than profile 2 ::: dom/media/VideoUtils.cpp:307 (Diff revision 2) > + if (aBitDepth != 8 && aBitDepth != 10 && aBitDepth != 12) { > + // Invalid bitDepth: > + return false; > + } > + > + if (fieldsCount < 3) { you've already tested that above ::: dom/media/VideoUtils.cpp:317 (Diff revision 2) > + // chromaSubsampling should be 0,1,2,3...4~7 are reserved. > + if (aChromaSubsampling > 3) { > + return false; > + } > + > + if (fieldsCount < 4) { if fieldsCount == 3 as it can't be anything else ::: dom/media/VideoUtils.cpp:325 (Diff revision 2) > + } > + > + // It is an integer that is defined by the "Colour primaries" > + // section of ISO/IEC 23001-8:2016 Table 2. > + // We treat reserved value as false case. > + const auto &primaryId = aColorSpace.mPrimaryId; const auto& ::: dom/media/VideoUtils.cpp:335 (Diff revision 2) > + if (primaryId > 12 && primaryId < 22) { > + // 13~21 are reserved values. > + return false; > + } > + > + if (fieldsCount < 5) { if fieldsCount == 4 ::: dom/media/VideoUtils.cpp:349 (Diff revision 2) > + if (transferId == 0 || transferId == 3 || transferId > 18) { > + // reserved value. > + return false; > + } > + > + if (fieldsCount < 6) { == 5 and so forth ::: dom/media/VideoUtils.cpp:376 (Diff revision 2) > + > + // videoFullRangeFlag indicates the black level and range of the luma and > + // chroma signals. 0 = legal range (e.g. 16-235 for 8 bit sample depth); > + // 1 = full range (e.g. 0-255 for 8-bit sample depth). > + const auto &rangeId = aColorSpace.mRangeId; > + if (rangeId > 1) { return rangeId <= 1;
Attachment #8918167 - Flags: review?(jyavenard) → review+
Attachment #8918167 - Flags: review?(gsquelart)
Attachment #8918168 - Flags: review?(jyavenard) → review?(gsquelart)
I suggest you should add a test for this kind of new codec string.
will add a gtest in Part 3 , thanks.
Comment on attachment 8918168 [details] Bug 1407919 - Part2 - Use the helper function to tell if it is a vp8/9 codec string. https://reviewboard.mozilla.org/r/189030/#review194408 ::: dom/media/VideoUtils.cpp:663 (Diff revision 6) > + uint8_t level = 0; > + uint8_t bitDepth = 0; > return aCodec.EqualsLiteral("vp8") || > - aCodec.EqualsLiteral("vp8.0"); > + aCodec.EqualsLiteral("vp8.0") || > + (StartsWith(NS_ConvertUTF16toUTF8(aCodec), "vp08") && > + ExtractVPXCodecDetails(aCodec, profile, level, bitDepth)); You don't actually use the extracted values. If you're never going to use them, you could simplify both patches by not outputting values, in which case you could rename the function something like `CheckVPXCodecDetails`.
Attachment #8918168 - Flags: review?(gsquelart) → review+
I'll review P1 after the current issues have been resolved.
You also need to properly handle what to do with those decoded values. In particular handling the requested bit depth. Not all system can play it... This is to be done in MP4Decoder.cpp. pass the bit depth value to VideoInfo::mBitDepth and then amend PDM::Supports() to properly handle that flag (only the ffvpx decoder supports, it and only when the compositor isn't D3D11)
Review board shows r+ for Part2, but bugzilla did not show(don't know why), but I found that I should modify the commit message from r?jya to gerald. Hi jya, Per comment 15 said, Should I add Part4 to handle the case you mentioned? If so, I'm lack of experience to deal with that, may need your help and feedback when I upload the Part4. Many thanks.
Flags: needinfo?(jyavenard)
Comment on attachment 8918168 [details] Bug 1407919 - Part2 - Use the helper function to tell if it is a vp8/9 codec string. carry r+
Attachment #8918168 - Flags: review+
Comment on attachment 8918168 [details] Bug 1407919 - Part2 - Use the helper function to tell if it is a vp8/9 codec string. what a bug...
Attachment #8918168 - Flags: superreview+
Attachment #8918168 - Flags: superreview+ → review+
Comment on attachment 8918168 [details] Bug 1407919 - Part2 - Use the helper function to tell if it is a vp8/9 codec string. https://reviewboard.mozilla.org/r/189030/#review194408 > You don't actually use the extracted values. > If you're never going to use them, you could simplify both patches by not outputting values, in which case you could rename the function something like `CheckVPXCodecDetails`. Per comment 15 said, it seems I need to use the extracted values to do some check. I will keep the naming. Thank you.
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review194446 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 2) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 2) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 2) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 2) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 2) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 2) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review194448 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 1) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 1) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 1) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 1) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 1) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 1) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review194450 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 3) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 3) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 3) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 3) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 3) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 3) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
The static analysis blamed on the google's GTEST code not mine. So I will ignore those.
I still cannot clearly get the full picture of what should I do for comment 15. But I think it could be done in another bug. I'd prefer to land these three parts and create a new bug for comment 15. Hi jya, do you agree with above comment? thank you. In another hand, we plan to land this bug and ask netflix to provide some information about Bug 1408302. Alastor is looking into Bug 1408302 now and may need more information from Netflix after this bug being landed.
Flags: needinfo?(jyavenard)
^^^^^^^^^^^^^^^^^^^^^ hi jya, please see above comment, thanks.
Flags: needinfo?(jyavenard)
(In reply to James Cheng[:JamesCheng] from comment #34) > ^^^^^^^^^^^^^^^^^^^^^ > hi jya, please see above comment, thanks. adding knowledge on weither we support bitdepth > 8 is trivial, I don't see why you couldn't do it right away. what would be gain by postponing for say 1 hour (the time it takes to write that change). If you don't, YouTube will start sending you HDR video which we don't play on windows. So no, you can't push those patches as is, you will break YouTube with some videos for people on Windows.
Flags: needinfo?(jyavenard)
I've added Par4 and 5 for comment 15's indication. I'm not sure I understood it correctly... Hi jya, By comment 35, Why did you say if I don't take care of the bit depth, I will break the Youtube? Do you mean that Youtube start to use this kind of codec parameter string starting with vp09 for their HDR content and I will reply we're supporting? Thanks, and please correct my patch.
Flags: needinfo?(jyavenard)
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review194950 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8918930 [details] Bug 1407919 - Part4 - Add mBitDepth field into VideoInfo. https://reviewboard.mozilla.org/r/189834/#review195002 commit message doesnt describe what the change does. you arent just adding mBitDepth here...
Attachment #8918930 - Flags: review?(jyavenard) → review-
Comment on attachment 8918930 [details] Bug 1407919 - Part4 - Add mBitDepth field into VideoInfo. https://reviewboard.mozilla.org/r/189834/#review195002 so, please, split this patch in two
Attachment #8918931 - Flags: review?(jyavenard) → review?(gsquelart)
Comment on attachment 8918931 [details] Bug 1407919 - Part7 - Check bit depth in PDM::Supports. https://reviewboard.mozilla.org/r/189836/#review195006 ::: dom/media/platforms/ffmpeg/FFmpegDecoderModule.h:87 (Diff revision 1) > + DecoderDoctorDiagnostics* aDiagnostics) const override > + { > + // We don't support bitDepth > 8 when compositor backend is D3D11. > + if (gfxPlatform::GetPlatform()->GetCompositorBackend() == > + mozilla::layers::LayersBackend::LAYERS_D3D11) { > + return aBitDepth == 8 ? true : false; return aBitDepth == 8
(In reply to James Cheng[:JamesCheng] from comment #41) > I've added Par4 and 5 for comment 15's indication. I'm not sure I understood > it correctly... > > Hi jya, > By comment 35, > Why did you say if I don't take care of the bit depth, I will break the > Youtube? Because if you tell YouTube you support HDR, they can serve you HDR content. Which would then cause a decoding error. > Do you mean that Youtube start to use this kind of codec parameter string > starting with vp09 for their HDR content and I will reply we're supporting? Yes, YouTube uses that exact same mime parameter to determine if we can do HDR or not. > > Thanks, and please correct my patch. I won’t correct your patch sorry.
Flags: needinfo?(jyavenard)
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review195256 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Hi matt, For the patch Part7, I would like to ask for your confirmation that the way I use to check if the compositor backend is D3D11. if (gfxPlatform::GetPlatform()->GetCompositorBackend() == mozilla::layers::LayersBackend::LAYERS_D3D11) { It is OK? Thank you. Hi gerald, Please review those patches. Thanks.
Comment on attachment 8918931 [details] Bug 1407919 - Part7 - Check bit depth in PDM::Supports. https://reviewboard.mozilla.org/r/189836/#review195310 ::: dom/media/platforms/PlatformDecoderModule.h:177 (Diff revision 3) > + // Indicates if the PlatformDecoderModule supports decoding of aBitDepth. > + // Should override this method when the platform can support bitDepth > 8. > + virtual bool SupportsBitDepth(const uint8_t aBitDepth, > + DecoderDoctorDiagnostics* aDiagnostics) const > + { > + if (aBitDepth != 8) { return aBitDepth == 8; ::: dom/media/platforms/PlatformDecoderModule.h:189 (Diff revision 3) > virtual bool > Supports(const TrackInfo& aTrackInfo, > DecoderDoctorDiagnostics* aDiagnostics) const > { > - // By default, fall back to SupportsMimeType with just the MIME string. > - // (So PDMs do not need to override this method -- yet.) > + const auto videoInfo = aTrackInfo.GetAsVideoInfo(); > + bool isSupportedBitDepth = should distinguish between audio track and video track. otherwise it seems that we care about audio bit depth (which makes no sense) or change the variable name. otherwise test can be better written as: bool isSupportedBitDepth = !videoInfo || SupportsBitDepth(videoInfo->mBitDepth, aDiagnostics); ::: dom/media/platforms/ffmpeg/FFmpegDecoderModule.h:85 (Diff revision 3) > > + bool SupportsBitDepth(const uint8_t aBitDepth, > + DecoderDoctorDiagnostics* aDiagnostics) const override > + { > + // We don't support bitDepth > 8 when compositor backend is D3D11. > + if (gfxPlatform::GetPlatform()->GetCompositorBackend() == this test could be better written as: aBitDepth == 8 || gfxPlatform::GetPlatform()->GetCompositorBackend() != mozilla::layers::LayersBackend::LAYERS_D3D11;
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review195316 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8919131 [details] Bug 1407919 - Part5 - Extract bit depth information from codec parameter string into VideoInfo::mBitDepth. https://reviewboard.mozilla.org/r/190042/#review195318 ::: commit-message-8873b:1 (Diff revision 2) > +Bug 1407919 - Part5 - Extract bit depth information from codec parameter string into VideoInfo::mBitmBitDepth. r?jya VideoInfo::mBitDepth
Attachment #8919131 - Flags: review?(jyavenard) → review+
Comment on attachment 8918930 [details] Bug 1407919 - Part4 - Add mBitDepth field into VideoInfo. https://reviewboard.mozilla.org/r/189834/#review195320 ::: dom/media/MediaInfo.h:220 (Diff revision 2) > , mStereoMode(StereoMode::MONO) > , mImage(aSize) > , mCodecSpecificConfig(new MediaByteBuffer) > , mExtraData(new MediaByteBuffer) > , mRotation(kDegree_0) > + , mBitDepth(8) use C++ initializer. so uint8_t mBitDepth = 8 below
Attachment #8918930 - Flags: review?(jyavenard) → review+
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review195332 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review195358 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 4) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 4) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8918931 [details] Bug 1407919 - Part7 - Check bit depth in PDM::Supports. https://reviewboard.mozilla.org/r/189836/#review195560 ::: dom/media/platforms/ffmpeg/FFmpegDecoderModule.h:87 (Diff revision 5) > + bool SupportsBitDepth(const uint8_t aBitDepth, > + DecoderDoctorDiagnostics* aDiagnostics) const override > + { > + // We don't support bitDepth > 8 when compositor backend is D3D11. > + return (aBitDepth == 8) || > + (gfxPlatform::GetPlatform()->GetCompositorBackend() != The backend on gfxPlatform is for diagnostic purposes only, we can't use it here. The compositor backend is per-window, so we need to know which window we're going to be rendering to before we can know which compositor we'll get. The 'KnowsCompositor' iterface is what we need, but I don't know if we can easily get that for the PDM. CreateDecoder gets one as part of the CreateDecoderParams, but that's probably too late for this. Can we just add support for D3D11 and not have this problem?
Attachment #8918931 - Flags: review?(matt.woodrow) → review-
Comment on attachment 8918167 [details] Bug 1407919 - Part1 - Add an extraction function to parse the RFC-6381 VP9 codec string. https://reviewboard.mozilla.org/r/189028/#review195580 ::: dom/media/VideoUtils.cpp:253 (Diff revision 6) > + int fieldsCount = 0; > + nsresult rv; > + for (fieldsCount = 0; fieldsItr != splitter.end(); ++fieldsItr, ++fieldsCount) { No need to set `fieldsCount` to 0 again. ::: dom/media/VideoUtils.cpp:260 (Diff revision 6) > + for (fieldsCount = 0; fieldsItr != splitter.end(); ++fieldsItr, ++fieldsCount) { > + if (fieldsCount > 7) { > + // No more than 8 fields are expected. > + return false; > + } > + *(fields[fieldsCount]) = static_cast<uint8_t>(PromiseFlatString((*fieldsItr)).ToInteger(&rv, 10)); This looks longer than 80 columns, have you run `./mach clang-format`?
Attachment #8918167 - Flags: review?(gsquelart) → review+
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review195602 ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:77 (Diff revision 4) > + uint8_t profile = 0; > + uint8_t level = 0; > + uint8_t bitDepth = 0; > + uint8_t chromaSubsampling = 0; > + VideoColorSpace colorSpace; > + auto data = u"vp09.01.11.08"; For completeness, could you please add a test with all 8 values?
Attachment #8918274 - Flags: review?(gsquelart) → review+
Comment on attachment 8919176 [details] Bug 1407919 - Part6 - Check bit depth in WebMDecoder to determine if we support HDR. https://reviewboard.mozilla.org/r/190088/#review195608 ::: dom/media/webm/WebMDecoder.cpp:50 (Diff revision 2) > + } > + if (IsVP8CodecString(codec)) { I think you can add an `else` in between, so that you don't test for VP8 if you already know it's VP9.
Attachment #8919176 - Flags: review?(gsquelart) → review+
Comment on attachment 8918931 [details] Bug 1407919 - Part7 - Check bit depth in PDM::Supports. https://reviewboard.mozilla.org/r/189836/#review195612 r- for now, so I can re-review after you address Matt's comment. ::: dom/media/platforms/PlatformDecoderModule.h:176 (Diff revision 5) > - // By default, fall back to SupportsMimeType with just the MIME string. > - // (So PDMs do not need to override this method -- yet.) > - return SupportsMimeType(aTrackInfo.mMimeType, aDiagnostics); > + const auto videoInfo = aTrackInfo.GetAsVideoInfo(); > + bool isSupportedBitDepth = > + !videoInfo || SupportsBitDepth(videoInfo->mBitDepth, aDiagnostics); > + return SupportsMimeType(aTrackInfo.mMimeType, aDiagnostics) && > + isSupportedBitDepth; You're computing isSupportedBitDepth first, even though it may not actually be used if SupportsMimeType fails. So I would suggest you reorder things, e.g.: if (!SupportsMimeType(...)) { return false; } videoInfo = ...; return !videoInfo || SupportsBitDepth(...); ::: dom/media/platforms/PlatformDecoderModule.h:193 (Diff revision 5) > friend class PDMFactory; > friend class dom::RemoteDecoderModule; > friend class EMEDecoderModule; > > + // Indicates if the PlatformDecoderModule supports decoding of aBitDepth. > + // Should override this method when the platform can support bitDepth > 8. '... can support bitDepth != 8' to match the code. ::: dom/media/platforms/PlatformDecoderModule.h:197 (Diff revision 5) > + // We only support bitDepth > 8 for ffvpx with non-D3D11 compositor backend. > + return aBitDepth == 8; This comment tells us about something about code in other files, that could one day change! (And I'm not sure it's even correct as it is.) The point of 'virtual' is to let derived classes decide what to do in non-default cases. The comment above the function already says "Should override this method when the platform can support bitDepth > 8.", so I don't think you need the fragile extra comment.
Attachment #8918931 - Flags: review?(gsquelart) → review-
(In reply to Matt Woodrow (:mattwoodrow) from comment #66) > Comment on attachment 8918931 [details] > Bug 1407919 - Part7 - Check bit depth in PDM::Supports. > > https://reviewboard.mozilla.org/r/189836/#review195560 > > ::: dom/media/platforms/ffmpeg/FFmpegDecoderModule.h:87 > (Diff revision 5) > > + bool SupportsBitDepth(const uint8_t aBitDepth, > > + DecoderDoctorDiagnostics* aDiagnostics) const override > > + { > > + // We don't support bitDepth > 8 when compositor backend is D3D11. > > + return (aBitDepth == 8) || > > + (gfxPlatform::GetPlatform()->GetCompositorBackend() != > > The backend on gfxPlatform is for diagnostic purposes only, we can't use it > here. > > The compositor backend is per-window, so we need to know which window we're > going to be rendering to before we can know which compositor we'll get. > > The 'KnowsCompositor' iterface is what we need, but I don't know if we can > easily get that for the PDM. > > CreateDecoder gets one as part of the CreateDecoderParams, but that's > probably too late for this. > > Can we just add support for D3D11 and not have this problem? Thanks for review and feedback. I ran the code on Windows and it returned LayersBackend::LAYERS_NONE to me(but about:support seems used the same API, but it can get the right D3D11 compositor) Anyway, I shouldn't invoke this API to get layers backend here.... And I cannot find anyway to get a nsIDocument object in this static method like http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/media/MediaDecoder.cpp#1062 did, so I have no way to ask for the backend type in this function. Hi jya, Do you agree that I reject `bitDepth > 8` no matter what layer backend is? If you agree, I would like to ask if there is an existing bug that intends to solve this problem(I could leave a TODO comment.). I saw bug 1215089 comment 30 and it seems we need graphics guys to deal with that... Thank you.
Flags: needinfo?(jyavenard)
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review195818 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 5) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 5) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8918931 [details] Bug 1407919 - Part7 - Check bit depth in PDM::Supports. https://reviewboard.mozilla.org/r/189836/#review195826 LGTM, assuming you get an OK for your comment 71.
Attachment #8918931 - Flags: review?(gsquelart) → review+
(In reply to James Cheng[:JamesCheng] from comment #71) > ( > Hi jya, > > Do you agree that I reject `bitDepth > 8` no matter what layer backend is? No. For now just return true if on Linux or Mac, false everywhere else.
Flags: needinfo?(jyavenard)
Comment on attachment 8918274 [details] Bug 1407919 - Part3 - Add a gtest for testing the extraction function. https://reviewboard.mozilla.org/r/189112/#review196122 C/C++ static analysis found 6 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 5) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:19 (Diff revision 5) > +{ > + const char16_t* const mCodecParameterString; > + const bool mExpectedValue; > +}; > + > +TEST(ExtractVPXCodecDetails, TestDataLength) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:40 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestInputData) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] ::: dom/media/mediasource/gtest/TestExtractVPXCodecDetails.cpp:71 (Diff revision 5) > + bool result = ExtractVPXCodecDetails(nsString(data.mCodecParameterString), profile, level, bitDepth); > + EXPECT_EQ(result, data.mExpectedValue) << NS_ConvertUTF16toUTF8(data.mCodecParameterString).get(); > + } > +} > + > +TEST(ExtractVPXCodecDetails, TestParsingOutput) { Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8918931 [details] Bug 1407919 - Part7 - Check bit depth in PDM::Supports. https://reviewboard.mozilla.org/r/189836/#review196132 Still r+ for the code, but I'd like :jya to confirm that it matches what he requested in comment 81.
Jean-Yves, could you please confirm that the new code in FFmpegDecoderModule.h in part 7 ( https://reviewboard.mozilla.org/r/189836/diff/7/#2 ) corresponds to what you requested in comment 81? (Can't find a way to reset the 'r?' flag in MozReview)
Flags: needinfo?(jyavenard)
Comment on attachment 8918931 [details] Bug 1407919 - Part7 - Check bit depth in PDM::Supports. https://reviewboard.mozilla.org/r/189836/#review196140
Attachment #8918931 - Flags: review+
Try looks good with my change. https://treeherder.mozilla.org/#/jobs?repo=try&revision=da185296909513c0f3d82c185b4f3befb1b2e896 Thank you for jya and gerald's guidance and review.
Flags: needinfo?(jyavenard)
Pushed by jacheng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a5a793805ac8 Part1 - Add an extraction function to parse the RFC-6381 VP9 codec string. r=gerald,jya https://hg.mozilla.org/integration/autoland/rev/0c390da6cce5 Part2 - Use the helper function to tell if it is a vp8/9 codec string. r=gerald https://hg.mozilla.org/integration/autoland/rev/589c6929104c Part3 - Add a gtest for testing the extraction function. r=gerald https://hg.mozilla.org/integration/autoland/rev/2fc247fa959b Part4 - Add mBitDepth field into VideoInfo. r=jya https://hg.mozilla.org/integration/autoland/rev/85fbdfbf84c0 Part5 - Extract bit depth information from codec parameter string into VideoInfo::mBitDepth. r=jya https://hg.mozilla.org/integration/autoland/rev/2d9ead16eb7d Part6 - Check bit depth in WebMDecoder to determine if we support HDR. r=gerald https://hg.mozilla.org/integration/autoland/rev/d6e6399ea965 Part7 - Check bit depth in PDM::Supports. r=gerald,jya
Blocks: 1410022
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: