Closed Bug 1291742 Opened 8 years ago Closed 8 years ago

Report MP4 videos with YUV444 format as unsupported

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Platform:
x86
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: alice0775, Assigned: JamesCheng)

References

Details

(Whiteboard: DUPEME,[parity-Chrome])

Attachments

(3 files, 1 obsolete file)

MFR+MDSM.log
172.86 KB, text/x-log
Details
CheckVideoSupportedWhenCreateDecoder
3.96 KB, patch
jya
: feedback+
Details | Diff | Splinter Review
Bug 1291742 - Report MP4 videos with YUV444 format as unsupported.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Google Chrome works as expected STR 1. Open attachment 8777324 [details] in Bug 1291701. Actual Results: No playback Expected Results: playback
Plays on Linux. Broken on Windows.
Priority: -- → P1
Assignee: nobody → jacheng
If this issue is not that urgent. I want to give it a shot.
James, FYI. Broken on Mac as well. Great to see you like to take on this bug! No worries. You always can get help from Benjamin or JW if you need any.
I will start to investigate why we got "No Sample" in MDSM. The video sink cannot be started normally as attachment. [MediaPlayback #1]: D/MediaFormatReader MediaFormatReader(fa26000)::OnDemuxFailed: Failed to demux video, failure:1 [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(fa26000)::ScheduleUpdate: SchedulingUpdate(Video) [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(fa26000)::Update: Processing update for Video [MediaPlayback #1]: D/MediaFormatReader MediaFormatReader(fa26000)::DrainDecoder: Requesting Video decoder to drain [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(fa26000)::NotifyInputExhausted: Decoder has requested more Video data [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(fa26000)::ScheduleUpdate: SchedulingUpdate(Video) [MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(fa26000)::NotifyDrainComplete: Video [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(fa26000)::Update: Processing update for Video [MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(fa26000)::Update: Rejecting Video promise: EOS [Main Thread]: D/MediaDecoder Decoder=b9a09c0 NotifyDownloadEnded, status=0 [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(fa26000)::Update: Update(Video) ni=0 no=1 ie=1, in:115 out:0 qs=115 pending:0 waiting:0 ahead:1 sid:4294967295 [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(fa26000)::Update: No need for additional input (pending:0) [MediaPlayback #1]: D/MediaDecoder StartTimeRendezvous=9817670 SampleType(1) Has no samples.
Attached file MFR+MDSM.log —
The failed case log on windows.
Hi jya, While playing attachment 8777324 [details] on windows. The [1] will always return MF_E_TRANSFORM_NEED_MORE_INPUT and notify by [2] for more demuxed data until it met EOS. (log is in attachment 8779184 [details] ) The chrome browser also use the WMF for decoding...but the video plays normally in Chrome. Do you have any idea why this video source is such special that results in this issue? Could you please provide me some hints or tools that I can do further investigation? Is there any recommended tool that is useful to analyze the video content? Thank you very much. [1] https://dxr.mozilla.org/mozilla-central/rev/e78975b53563d80c99ebfbdf8a9fbf6b829a8a48/dom/media/platforms/wmf/MFTDecoder.cpp#211 [2] https://dxr.mozilla.org/mozilla-central/rev/e78975b53563d80c99ebfbdf8a9fbf6b829a8a48/dom/media/platforms/wmf/WMFMediaDataDecoder.cpp#153
Flags: needinfo?(jyavenard)
WMF doesn't support yuv444p (High 4:4:4). My guess is that Chrome default to ffmpeg for those videos.
Flags: needinfo?(jyavenard)
From https://msdn.microsoft.com/en-us/library/windows/desktop/dd797815(v=vs.85).aspx Profiles/Levels: Baseline, Main, and High profiles, up to level 5.1. (See ITU-T H.264 specification for details.) Chroma Formats: 4:2:0 chroma or monochrome Note that it won't play on mac either. Only the FFmpeg decoder has a yuv444 decoder.
(In reply to James Cheng[:JamesCheng] from comment #7) > The chrome browser also use the WMF for decoding...but the video plays > normally in Chrome. doubtful > Is there any recommended tool that is useful to analyze the video content? ffmpeg/ffprobe is what I use. for looking at the container: mp4dump
James, we could probably report those videos as broken or unsupported by checking the SPS NAL. We have all the bricks available to perform this task. Ping me on IRC for more details.
Thanks jya for addressing the key point. I transcode to YUV420 and it can play on Windows.... I will ping you on IRC to know how can I do next! Much appreciated.
Hi jya, Please take a look at this patch, I used the |chroma_format_idc| value to check YUV444 case. Naively, I want a helper let can append some rules to check all the conditions satisfied and add rules when needed. If you think it is unnecessary, I will remove it and just check the condition directly in CreateVideoDecoder. Thank you for your help.
Attachment #8780250 - Flags: feedback?(jyavenard)
Summary: A video would not play → Report MP4 videos with YUV444 format as unsupported
Comment on attachment 8780250 [details] [diff] [review] CheckVideoSupportedWhenCreateDecoder Review of attachment 8780250 [details] [diff] [review]: ----------------------------------------------------------------- very nice...
Attachment #8780250 - Flags: feedback?(jyavenard) → feedback+
(In reply to James Cheng[:JamesCheng] from comment #13) > Thanks jya for addressing the key point. > > I transcode to YUV420 and it can play on Windows.... what do you mean by transcoding? something to do on the fly or using an external utility? Currently on linux it will play as ffmpeg will perform the conversion automatically, however as it will not play on either windows or mac; what about refusing those outright to ensure consistency across platforms? Should also return this to the DecoderDoctor. Reporting the video as broken is misleading, as to me it's not broken. It just happens that our decoder can't do it
Comment on attachment 8781075 [details] Bug 1291742 - [Part2] Report to decoder doctor. (In reply to Jean-Yves Avenard [:jya] from comment #16) > (In reply to James Cheng[:JamesCheng] from comment #13) > > Thanks jya for addressing the key point. > > > > I transcode to YUV420 and it can play on Windows.... > what do you mean by transcoding? something to do on the fly or using an > external utility? > > Currently on linux it will play as ffmpeg will perform the conversion > automatically, however as it will not play on either windows or mac; what > about refusing those outright to ensure consistency across platforms? > > Should also return this to the DecoderDoctor. Reporting the video as broken > is misleading, as to me it's not broken. It just happens that our decoder > can't do it Hi jya, I'm not familiar with Decoder Doctor, I guess I should do some judgment like this WIP patch and I need to add some methods into Decoder Doctor to record the unsupported case, right? Could you please give me some advice if I'm in the right direction? Thank you Should I need to
Attachment #8781075 - Flags: review?(jyavenard) → feedback?(jyavenard)
Hi jya, In part1, I also modified some enum type into enum class type. Please take a look. Thank you.
Comment on attachment 8781074 [details] Bug 1291742 - Report MP4 videos with YUV444 format as unsupported. https://reviewboard.mozilla.org/r/71572/#review69774 <p>Most of the changes in this patch is unrelated to the comment. Please put another patch.</p> <p>We've always stated that we want consistency across platform.</p> <p>Please move this inside the PlatformDecoderModule and remove the need to modify the PlatformDecoderModule or create a new source file.</p> ::: dom/media/platforms/PlatformDecoderModule.h:112 (Diff revision 1) > > // Indicates if the PlatformDecoderModule supports decoding of aMimeType. > virtual bool SupportsMimeType(const nsACString& aMimeType, > DecoderDoctorDiagnostics* aDiagnostics) const = 0; > > - enum ConversionRequired { > + enum class ConversionRequired : uint8_t { this is out of scope for this patch... please create another bug / patch ::: dom/media/platforms/PlatformDecoderModule.h:183 (Diff revision 1) > + }; // SupportChecker > + > + SupportChecker mSupportChecker; > }; > > -enum MediaDataDecoderError { > +enum class MediaDataDecoderError : uint8_t{ same as the conversion bit ::: dom/media/platforms/PlatformDecoderModule.cpp:25 (Diff revision 1) > +PlatformDecoderModule::SupportChecker::AddCheckVideoFormat(const VideoInfo& aVideoConfig) > +{ > + // TODO: ffmpeg can support this format. > + // Decide if ffmpeg should have the same > + // behavior as others for consistency. > +#ifndef MOZ_FFMPEG why would this only be used if FFMpeg isn't built? it's totally separate from FFmpeg; and FFmpeg can be built on mac too.
Attachment #8781074 - Flags: review?(jyavenard) → review-
Attachment #8781075 - Flags: feedback?(jyavenard)
H(In reply to Jean-Yves Avenard [:jya] from comment #21) > Comment on attachment 8781074 [details] > Bug 1291742 - [Part1] Check MP4 videos with YUV444 format as unsupported. > > https://reviewboard.mozilla.org/r/71572/#review69774 > > <p>Most of the changes in this patch is unrelated to the comment. Please put > another patch.</p> > <p>We've always stated that we want consistency across platform.</p> > <p>Please move this inside the PlatformDecoderModule and remove the need to > modify the PlatformDecoderModule or create a new source file.</p> > > ::: dom/media/platforms/PlatformDecoderModule.h:112 > (Diff revision 1) > > > > // Indicates if the PlatformDecoderModule supports decoding of aMimeType. > > virtual bool SupportsMimeType(const nsACString& aMimeType, > > DecoderDoctorDiagnostics* aDiagnostics) const = 0; > > > > - enum ConversionRequired { > > + enum class ConversionRequired : uint8_t { > > this is out of scope for this patch... please create another bug / patch > Sorry, I create a new Bug 1295920 to do this. > ::: dom/media/platforms/PlatformDecoderModule.h:183 > (Diff revision 1) > > + }; // SupportChecker > > + > > + SupportChecker mSupportChecker; > > }; > > > > -enum MediaDataDecoderError { > > +enum class MediaDataDecoderError : uint8_t{ > > same as the conversion bit > > ::: dom/media/platforms/PlatformDecoderModule.cpp:25 > (Diff revision 1) > > +PlatformDecoderModule::SupportChecker::AddCheckVideoFormat(const VideoInfo& aVideoConfig) > > +{ > > + // TODO: ffmpeg can support this format. > > + // Decide if ffmpeg should have the same > > + // behavior as others for consistency. > > +#ifndef MOZ_FFMPEG > > why would this only be used if FFMpeg isn't built? > > it's totally separate from FFmpeg; and FFmpeg can be built on mac too. Sorry for that, I adjust this in the new patch. Thank you.
Attachment #8781075 - Flags: review?(jyavenard)
Comment on attachment 8781074 [details] Bug 1291742 - Report MP4 videos with YUV444 format as unsupported. https://reviewboard.mozilla.org/r/71572/#review70092 As mentioned in my previous review.. Please perform such test in the PDMFactory so that all decoders are equals ::: dom/media/platforms/PlatformDecoderModule.h:14 (Diff revision 2) > #include "MediaDecoderReader.h" > +#include "mozilla/Function.h" > #include "mozilla/MozPromise.h" > #include "mozilla/layers/LayersTypes.h" > -#include "nsTArray.h" > #include "mozilla/RefPtr.h" if you are to re-order alphabetically, mozilla/RefPtr should be before mozilla/layers ::: dom/media/platforms/PlatformDecoderModule.h:119 (Diff revision 2) > kNeedAVCC, > kNeedAnnexB, > }; > > + enum class ColorFormat : uint8_t { > + kYUV400 = 0, you don't need to set values for each here, especially with enum class as they can't be converted to numerical value
Attachment #8781074 - Flags: review?(jyavenard) → review-
Comment on attachment 8781075 [details] Bug 1291742 - [Part2] Report to decoder doctor. https://reviewboard.mozilla.org/r/71574/#review70094 i don't see the point of a commit that does effectively nothing but add a TODO.
Attachment #8781075 - Flags: review-
Attachment #8781074 - Attachment is obsolete: true
Attachment #8781075 - Attachment is obsolete: true
Attachment #8781075 - Flags: feedback?(jyavenard)
Hi jya, Based on your comments, Please help to review the new one. Thank you very much!
Comment on attachment 8781074 [details] Bug 1291742 - Report MP4 videos with YUV444 format as unsupported. https://reviewboard.mozilla.org/r/71572/#review70632 ::: dom/media/DecoderDoctorDiagnostics.h:73 (Diff revision 3) > bool DidFFmpegFailToLoad() const { return mFFmpegFailedToLoad; } > > void SetGMPPDMFailedToStartup() { mGMPPDMFailedToStartup = true; } > bool DidGMPPDMFailToStartup() const { return mGMPPDMFailedToStartup; } > + > + void SetVideoFormatNotSupport() { mVideoFormatNotSupport = true; } m[Video|Audio]NoSupport or m[Video|Audio]NotSupported ::: dom/media/platforms/PDMFactory.h:63 (Diff revision 3) > > already_AddRefed<MediaDataDecoder> > CreateDecoderWithPDM(PlatformDecoderModule* aPDM, > const CreateDecoderParams& aParams); > > + enum class ColorFormat : uint8_t { { on the start of the next line, below enum. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes ::: dom/media/platforms/PDMFactory.h:74 (Diff revision 3) > + > + class SupportChecker > + { > + public: > + enum class Result : uint8_t { > + kSupported = 0, you don't need an initializer = 0 ::: dom/media/platforms/PDMFactory.h:88 (Diff revision 3) > + > + Result Check(); > + > + void Clear() { mCheckList.Clear(); } > + private: > + unecessary break of line ::: dom/media/platforms/PDMFactory.cpp:125 (Diff revision 3) > } > > already_AddRefed<MediaDataDecoder> > PDMFactory::CreateDecoder(const CreateDecoderParams& aParams) > { > + PR0(); what's that? ::: dom/media/platforms/PDMFactory.cpp:388 (Diff revision 3) > + mp4_demuxer::SPSData spsdata; > + // WMF H.264 Video Decoder and Apple ATDecoder > + // does not support YUV444 format. > + // For consistency, all decoders should be checked. > + if (!mp4_demuxer::H264::DecodeSPSFromExtraData(extraData, spsdata) || > + spsdata.chroma_format_idc == static_cast<uint8_t>(ColorFormat::kYUV444)) { this is not elegant IMHO, a enum class should have no numerical value. make a static const value indicating what YUV444 actually is (and it's also likely YUV422 won't be supported either)
Attachment #8781074 - Flags: review?(jyavenard) → review+
Attachment #8781074 - Flags: review?(gsquelart)
Comment on attachment 8781074 [details] Bug 1291742 - Report MP4 videos with YUV444 format as unsupported. https://reviewboard.mozilla.org/r/71572/#review70638 I'm concerned with the overall picture (6th issue below, you may want to look at it first). Keeping r? until you update this patch, or you convince me it's the right solution (in which case, you may want to add comments to explain that in the code.) Thank you. ::: dom/media/DecoderDoctorDiagnostics.h:73 (Diff revision 3) > + void SetVideoFormatNotSupport() { mVideoFormatNotSupport = true; } > + void SetAudioFormatNotSupport() { mAudioFormatNotSupport = true; } For consistency with other methods, please use the past tense: '...NotSupported'. ::: dom/media/DecoderDoctorDiagnostics.cpp:743 (Diff revision 3) > + if (mVideoFormatNotSupport) { > + s+= ", but video format did not support"; > + } > + if (mAudioFormatNotSupport) { > + s+= ", but audio format did not support"; > + } 'did not support' -> 'not supported' Also this change in GetDescription() will only show up in the terminal when MOZ_LOG contains "DecoderDoctor:5". It would probably be nice to show something in the web console. I'd understand if you didn't want to tackle that much in this bug -- it might be a significant amount of work! So please open a follow-up bug so that you and/or I can do that later on. ::: dom/media/platforms/PDMFactory.h:74 (Diff revision 3) > + > + class SupportChecker > + { > + public: > + enum class Result : uint8_t { > + kSupported = 0, And enum brace on new line please. ::: dom/media/platforms/PDMFactory.h:88 (Diff revision 3) > + > + Result Check(); > + > + void Clear() { mCheckList.Clear(); } > + private: > + and I would actually put a line break *before* 'private:'. ::: dom/media/platforms/PDMFactory.h:89 (Diff revision 3) > + Result Check(); > + > + void Clear() { mCheckList.Clear(); } > + private: > + > + nsTArray<mozilla::function<SupportChecker::Result()>> mCheckList; 'mCheckerList', as it's a list of checkers. :-) ::: dom/media/platforms/PDMFactory.cpp:175 (Diff revision 3) > + mSupportChecker.AddCheckMediaFormat(config); > + > + auto reason = mSupportChecker.Check(); Overall picture, if I understand correctly: You store mSupportChecker in the PDMFactory singleton, and then immediately run a check. So for each 'CreateDecoderWithPDM' call you may be adding a checker specific to the given parameters, and you will call it again and again in future calls for other (possibly-unrelated) decoders, is that really what you want? In fact these checkers keep references to things inside the TrackInfo, which may get destroyed, causing UAF later on. (Or am I missing something?) Instead, couldn't you just create a SupportChecker on the stack and use it right there? And really, couldn't you just directly use the code in AddCheckMediaFormat() and its embedded lambda, as a single plain PDMFactory method? (Unless you have some grand plans for these checkers later on.) And if you do that, a few other issues will just disappear too. ::: dom/media/platforms/PDMFactory.cpp:181 (Diff revision 3) > + if (reason == SupportChecker::Result::kVideoFormatNotSupported) > + diagnostics->SetVideoFormatNotSupport(); > + else if (reason == SupportChecker::Result::kAudioFormatNotSupported) { Add braces around if block, even when it's only one line. ::: dom/media/platforms/PDMFactory.cpp:369 (Diff revision 3) > +PDMFactory::SupportChecker::AddToCheckList(mozilla::function<SupportChecker::Result()> aChecker) > +{ > + mCheckList.AppendElement(aChecker); Please take aChecker as rvalue-ref (&&) and then mozilla::Move() it to AppendElement. (Always a good ideas when potentially taking lambda functions.) ::: dom/media/platforms/PDMFactory.cpp:375 (Diff revision 3) > +{ > + mCheckList.AppendElement(aChecker); > +} > + > +void > +PDMFactory::SupportChecker::AddCheckMediaFormat(const TrackInfo& aTrackConfig) I'd probably call this method 'AddMediaFormatChecker'. ::: dom/media/platforms/PDMFactory.cpp:385 (Diff revision 3) > + AddToCheckList( > + [mimeType, extraData]() { > + if (MP4Decoder::IsH264(mimeType)) { > + mp4_demuxer::SPSData spsdata; > + // WMF H.264 Video Decoder and Apple ATDecoder > + // does not support YUV444 format. 'does' -> 'do' ::: dom/media/platforms/PDMFactory.cpp:388 (Diff revision 3) > + mp4_demuxer::SPSData spsdata; > + // WMF H.264 Video Decoder and Apple ATDecoder > + // does not support YUV444 format. > + // For consistency, all decoders should be checked. > + if (!mp4_demuxer::H264::DecodeSPSFromExtraData(extraData, spsdata) || > + spsdata.chroma_format_idc == static_cast<uint8_t>(ColorFormat::kYUV444)) { and please left-align 'spsdata' under '!mp4_...' ::: dom/media/platforms/PDMFactory.cpp:400 (Diff revision 3) > +} > + > +PDMFactory::SupportChecker::Result > +PDMFactory::SupportChecker::Check() > +{ > + for (auto check : mCheckList) { 'auto&', to avoid copying function objects. And I'd probably call the variable 'checker'.
Comment on attachment 8781074 [details] Bug 1291742 - Report MP4 videos with YUV444 format as unsupported. https://reviewboard.mozilla.org/r/71572/#review70960 Thank you for the update. r+ after you fix a couple more nits: ::: dom/media/platforms/PDMFactory.h:63 (Diff revisions 3 - 4) > - enum class ColorFormat : uint8_t { > - kYUV400 = 0, > - kYUV420, > - kYUV422, > + static constexpr int kYUV400 = 0; > + static constexpr int kYUV420 = 1; > + static constexpr int kYUV422 = 2; > + static constexpr int kYUV444 = 3; You are comparing these constants to chroma_format_idc, which is a uint8_t, so I think these should be uint8_t as well -- if only to avoid potential warnings. ::: dom/media/platforms/PDMFactory.cpp:182 (Diff revisions 3 - 4) > + } > else if (reason == SupportChecker::Result::kAudioFormatNotSupported) { Closing '}' should be on the same line as 'else'.
Attachment #8781074 - Flags: review?(gsquelart) → review+
Thank you jya and gerald for the reviewing and feedback. (In reply to Gerald Squelart [:gerald] from comment #30) > Comment on attachment 8781074 [details] > Bug 1291742 - Report MP4 videos with YUV444 format as unsupported. > > https://reviewboard.mozilla.org/r/71572/#review70638 > > I'm concerned with the overall picture (6th issue below, you may want to > look at it first). > Keeping r? until you update this patch, or you convince me it's the right > solution (in which case, you may want to add comments to explain that in the > code.) Thank you. > > ::: dom/media/DecoderDoctorDiagnostics.h:73 > (Diff revision 3) > > + void SetVideoFormatNotSupport() { mVideoFormatNotSupport = true; } > > + void SetAudioFormatNotSupport() { mAudioFormatNotSupport = true; } > > For consistency with other methods, please use the past tense: > '...NotSupported'. > > ::: dom/media/DecoderDoctorDiagnostics.cpp:743 > (Diff revision 3) > > + if (mVideoFormatNotSupport) { > > + s+= ", but video format did not support"; > > + } > > + if (mAudioFormatNotSupport) { > > + s+= ", but audio format did not support"; > > + } > > 'did not support' -> 'not supported' > > Also this change in GetDescription() will only show up in the terminal when > MOZ_LOG contains "DecoderDoctor:5". > It would probably be nice to show something in the web console. > I'd understand if you didn't want to tackle that much in this bug -- it > might be a significant amount of work! > So please open a follow-up bug so that you and/or I can do that later on. > Sure, I will do this. I'm not familiar with the decoder doctor much that I cannot even enable or try to use it in my local environment. If this is not too complicated, I can try to work on it... But I would like to be mentored. > ::: dom/media/platforms/PDMFactory.h:74 > (Diff revision 3) > > + > > + class SupportChecker > > + { > > + public: > > + enum class Result : uint8_t { > > + kSupported = 0, > > And enum brace on new line please. > OK ~ but I took some code in DXR for reference, the enum coding style is not consistent actually. Some might add brace to new line but some might not. The Mozilla Coding Guide did not explicitly mention this. > ::: dom/media/platforms/PDMFactory.h:88 > (Diff revision 3) > > + > > + Result Check(); > > + > > + void Clear() { mCheckList.Clear(); } > > + private: > > + > > and I would actually put a line break *before* 'private:'. > > ::: dom/media/platforms/PDMFactory.h:89 > (Diff revision 3) > > + Result Check(); > > + > > + void Clear() { mCheckList.Clear(); } > > + private: > > + > > + nsTArray<mozilla::function<SupportChecker::Result()>> mCheckList; > > 'mCheckerList', as it's a list of checkers. :-) > > ::: dom/media/platforms/PDMFactory.cpp:175 > (Diff revision 3) > > + mSupportChecker.AddCheckMediaFormat(config); > > + > > + auto reason = mSupportChecker.Check(); > > Overall picture, if I understand correctly: > > You store mSupportChecker in the PDMFactory singleton, and then immediately > run a check. > So for each 'CreateDecoderWithPDM' call you may be adding a checker specific > to the given parameters, and you will call it again and again in future > calls for other (possibly-unrelated) decoders, is that really what you want? Thanks for reminding, I expected PDMFactory is not a singleton but PDMFactoryImpl is. So I did not consider the repeated calling case. Use it as a stack variable is much better, Thanks. > In fact these checkers keep references to things inside the TrackInfo, which > may get destroyed, causing UAF later on. (Or am I missing something?) > I noticed that so I captured by value to lambda function > Instead, couldn't you just create a SupportChecker on the stack and use it > right there? > And really, couldn't you just directly use the code in AddCheckMediaFormat() > and its embedded lambda, as a single plain PDMFactory method? > (Unless you have some grand plans for these checkers later on.) > And if you do that, a few other issues will just disappear too. > > ::: dom/media/platforms/PDMFactory.cpp:181 > (Diff revision 3) > > + if (reason == SupportChecker::Result::kVideoFormatNotSupported) > > + diagnostics->SetVideoFormatNotSupport(); > > + else if (reason == SupportChecker::Result::kAudioFormatNotSupported) { > > Add braces around if block, even when it's only one line. > > ::: dom/media/platforms/PDMFactory.cpp:369 > (Diff revision 3) > > +PDMFactory::SupportChecker::AddToCheckList(mozilla::function<SupportChecker::Result()> aChecker) > > +{ > > + mCheckList.AppendElement(aChecker); > > Please take aChecker as rvalue-ref (&&) and then mozilla::Move() it to > AppendElement. > (Always a good ideas when potentially taking lambda functions.) > Thank you, but how about |copy then move| instead of |r-value then move|? It can allow caller to pass a l-value mozilla::Function. > ::: dom/media/platforms/PDMFactory.cpp:375 > (Diff revision 3) > > +{ > > + mCheckList.AppendElement(aChecker); > > +} > > + > > +void > > +PDMFactory::SupportChecker::AddCheckMediaFormat(const TrackInfo& aTrackConfig) > > I'd probably call this method 'AddMediaFormatChecker'. > > ::: dom/media/platforms/PDMFactory.cpp:385 > (Diff revision 3) > > + AddToCheckList( > > + [mimeType, extraData]() { > > + if (MP4Decoder::IsH264(mimeType)) { > > + mp4_demuxer::SPSData spsdata; > > + // WMF H.264 Video Decoder and Apple ATDecoder > > + // does not support YUV444 format. > > 'does' -> 'do' > > ::: dom/media/platforms/PDMFactory.cpp:388 > (Diff revision 3) > > + mp4_demuxer::SPSData spsdata; > > + // WMF H.264 Video Decoder and Apple ATDecoder > > + // does not support YUV444 format. > > + // For consistency, all decoders should be checked. > > + if (!mp4_demuxer::H264::DecodeSPSFromExtraData(extraData, spsdata) || > > + spsdata.chroma_format_idc == static_cast<uint8_t>(ColorFormat::kYUV444)) { > > and please left-align 'spsdata' under '!mp4_...' > > ::: dom/media/platforms/PDMFactory.cpp:400 > (Diff revision 3) > > +} > > + > > +PDMFactory::SupportChecker::Result > > +PDMFactory::SupportChecker::Check() > > +{ > > + for (auto check : mCheckList) { > > 'auto&', to avoid copying function objects. > And I'd probably call the variable 'checker'.
(In reply to James Cheng[:JamesCheng] from comment #33) > (In reply to Gerald Squelart [:gerald] from comment #30) > > Also this change in GetDescription() will only show up in the terminal when > > MOZ_LOG contains "DecoderDoctor:5". > > It would probably be nice to show something in the web console. > > I'd understand if you didn't want to tackle that much in this bug -- it > > might be a significant amount of work! > > So please open a follow-up bug so that you and/or I can do that later on. > > > Sure, I will do this. > I'm not familiar with the decoder doctor much that I cannot even enable or > try to use it in my local environment. > If this is not too complicated, I can try to work on it... But I would like > to be mentored. Please open a bug for that, e.g. "Display unsupported a/v format warning in console from Decoder Doctor". Happy to mentor, and/or do it myself. > > And enum brace on new line please. > OK ~ but I took some code in DXR for reference, the enum coding style is not > consistent actually. I'm of the view that 'enum' is most closely related to 'class' (i.e., they're both *data* structures), and so for consistency should follow the same style. On the other hand, same-line braces are mostly found in *code* control statements: if {} { ... (But it's true that the coding style page is not clear about that.) > > In fact these checkers keep references to things inside the TrackInfo, which > > may get destroyed, causing UAF later on. (Or am I missing something?) > I noticed that so I captured by value to lambda function Good point, it would have worked (albeit inefficiently because of the singleton hoarding), sorry for the confusion. > > Please take aChecker as rvalue-ref (&&) and then mozilla::Move() it to > > AppendElement. > > (Always a good ideas when potentially taking lambda functions.) > Thank you, but how about |copy then move| instead of |r-value then move|? > It can allow caller to pass a l-value mozilla::Function. Actually, re-reading your code, I think you should use perfect forwarding inline in the class definition, e.g.: > template <class F> void AddToCheckList(F&& aChecker) { ...AppendElement(mozilla::Forward<T>(aChecker)); } This will move lambdas in the most efficient way (without creating an intermediate temporary mozilla::function), and would allow lvalue objects too but only copy them once.
Keywords: checkin-needed
Keywords: checkin-needed
Blocks: 1297003
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/766b607ad8f9 Report MP4 videos with YUV444 format as unsupported. r=gerald
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/766b607ad8f9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1300693
Blocks: 1300823

Sorry to dig out such an old bug, but I just ran into this and wonder if it should be re-evaluated. IE and classic Edge are no more, so on Windows Firefox is probably the only browser not playing these videos, similar on Linux, potentially MacOS as well.

Further more the current approach seems to also block downloads, which prevents people to use local players - that's certainly a bug.

Jean, do you mind having another look if it's still useful? If so, I'll create a bug for the download issue, but I'd prefer to see it fully removed.

P.S.: here is a link, might get updated at some point though: https://upload.video.fosdem.org/r/b8d0d06522d1da2c161222db7ac65deeb4e92c19d0e2f2aa622ebf0a2cd2acd5

Flags: needinfo?(jya-moz)

We actually support YUV444 format, it's AV_PIX_FMT_YUV444P:
https://searchfox.org/mozilla-central/rev/c1598f6d3edad19ccc53f53ab045d1d29835e1dd/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp#680

We may remove the check as it may be outdated.

I see, the problem is with h.264 only, VP9 and others should play fine.

I tested the video from https://bugzilla.mozilla.org/attachment.cgi?id=8777324 and it plays OK on Windows in Chrome/Edge, Firefox is the one who fails. I don't have MacOS so I can't test. So I think we should allow that now. I'll try to remove the check and rebuild Firefox on Windows with that.

Just removing the check on Windows does not play the media, try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dfc446824f8a301ccdc9cf30924d75b4ed60b33
I wonder what else needs to be done there. Works on Linux without any modifications as ffmpeg can play it.

IIUC ffmpeg is used for some file formats on Windows, so I guess what we'd have to do is what's already described in comment 8: move the check somewhere so WMF is not used for this formats and fall back to ffmpeg instead. Assuming that's legally possible, considering that h264 is involved.

(In reply to Robert Mader [:rmader] from comment #44)

IIUC ffmpeg is used for some file formats on Windows, so I guess what we'd have to do is what's already described in comment 8: move the check somewhere so WMF is not used for this formats and fall back to ffmpeg instead. Assuming that's legally possible, considering that h264 is involved.

unfortunately, that is not legally possible on Windows.
Firefox do not ship a ffmpeg with patent encumbered codec support.

Flags: needinfo?(jya-moz)

Does the OpenH264 GMP plugin have support for YUV444? (bug 1663844)
If a video has multiple sources, could it be made that HW decoding (all codecs) is tried first, then software decoding with free codecs and OpenH264 only as last resort before showing an error?

I wonder what Chrome/Edge uses on Windows to play that....is that VFW SW decoder or so?

There can be support in the GPU.

As it works under Chromium/Linux we should try to enable it for Firefox/Linux too.

Flags: needinfo?(stransky)

(In reply to Martin Stránský [:stransky] (ni? me) from comment #49)

As it works under Chromium/Linux we should try to enable it for Firefox/Linux too.

Filed as Bug 1898501.

Flags: needinfo?(stransky)
See Also: → 1898501
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: