Report MP4 videos with YUV444 format as unsupported

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Alice0775 White, Assigned: JamesCheng)

Tracking

Trunk
mozilla51
x86
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: DUPEME,[parity-Chrome])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
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
Broken on Edge.
(Assignee)

Updated

a year ago
Assignee: nobody → jacheng
(Assignee)

Comment 3

a year ago
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.
(Assignee)

Comment 5

a year ago
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.
(Assignee)

Comment 6

a year ago
Created attachment 8779184 [details]
MFR+MDSM.log

The failed case log on windows.
(Assignee)

Comment 7

a year ago
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
Duplicate of this bug: 1293243
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.
(Assignee)

Comment 13

a year ago
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.
(Assignee)

Comment 14

a year ago
Created attachment 8780250 [details] [diff] [review]
CheckVideoSupportedWhenCreateDecoder

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)

Updated

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

a year ago
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)
(Assignee)

Comment 20

a year ago
Hi jya, 

In part1, I also modified some enum type into enum class type.

Please take a look.

Thank you.

Comment 21

a year ago
mozreview-review
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-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8781075 - Flags: feedback?(jyavenard)
(Assignee)

Comment 24

a year ago
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.
(Assignee)

Updated

a year ago
Attachment #8781075 - Flags: review?(jyavenard)

Comment 25

a year ago
mozreview-review
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 26

a year ago
mozreview-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-
(Assignee)

Updated

a year ago
Attachment #8781074 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8781075 - Attachment is obsolete: true
Attachment #8781075 - Flags: feedback?(jyavenard)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

a year ago
Hi jya,

Based on your comments,

Please help to review the new one.

Thank you very much!

Comment 29

a year ago
mozreview-review
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 30

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 32

a year ago
mozreview-review
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+
(Assignee)

Comment 33

a year ago
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.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1297003
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 37

a year ago
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

Comment 38

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/766b607ad8f9
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

a year ago
Depends on: 1300693
Blocks: 1300823
You need to log in before you can comment on or make changes to this bug.