Closed Bug 1297003 Opened 8 years ago Closed 8 years ago

Store unsupported a/v format warning in MediaResult.

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng, Mentored)

References

Details

Attachments

(2 files)

Bug 1297003 - Part0- Fix nit.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1297003 - Part1-Store the error information in MediaResult while creating a/v decoders
58 bytes, text/x-review-board-request
mozbugz
: review+
Details 
Bug 1291742 will check the video / audio format before creating the decoder.

Once the decoder cannot be created due to media format, it should pass the warning message to the front-end through decoder doctor.

This is the following bug for Bug 1291742.
Hi gerald,

If you think this bug is not too complicated, maybe I can try to handle this bug.

Please mentor me once you have time :) 

Or just give me some directions that I can start to study.

Thank you!
Happy to mentor if I can.

Currently we only show one summary message after each run of analysis (in DecoderDoctorDocumentWatcher::ReportAnalysis).
But I think these new messages should be shown whenever we encounter the problem, separately from the normal analysis.

So I would suggest: (Don't look yet if you'd like to investigate by yourself first!)

- Add accessors for mVideo/AudioNotSupported in the header file.
- Create a utility method to send a message to the console, similar to the call to nsContentUtils::ReportToConsole in ReportAnalysis. After that, maybe ReportAnalysis could call this new method instead, if there's enough code in common.
- Add new strings in dom/locales/en-US/chrome/dom/dom.properties, after MediaNoDecoders.
- In DecoderDoctorDocumentWatcher::AddDiagnostics, add the code to test if audio/video is not supported, and send a console message then.
- In case this proves too noisy, we may want to make this conditional on some pref, e.g. "media.decoder-doctor.verbose" (which is already used elsewhere).

Good luck! Please contact me if you have any questions.
Assignee: nobody → jacheng
Mentor: gsquelart
Hi Mentor,

Thank you~ 

I will investigate first without seeing the suggestions.
James, FYI:
I'm currently working in DecoderDoctorDiagnostics.* in bug 1247056, which will implement part of the work needed here too.
So before you start working on this bug here, please wait until bug 1247056 lands, or contact me.
Depends on: 1247056
Hi Gerald,

Thanks for informing, I have not worked on this yet... , I will keep tracking bug 1247056.

Thank you!
Attachment #8796489 - Flags: review?(gsquelart)
Comment on attachment 8796489 [details]
Bug 1297003 - Part0- Fix nit.

https://reviewboard.mozilla.org/r/82338/#review81124

Thank you for fixing these nits.

I assume there are more parts coming to this bug, right?
Attachment #8796489 - Flags: review?(gsquelart) → review+
Yes, I will focus on this bug this week, thank you :)
Attachment #8797369 - Flags: review?(gsquelart)
Hi Gerald,

I'm not sure I'm in the right direction.

Please give me some feedback of this patch.

Here are some questions when I tried to verified the code

There is no DecoderDoctorDiagnostics passing to the API
http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/dom/media/MediaFormatReader.cpp#403,416

So it will get the null pointer without any diagnostic effect
http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/dom/media/platforms/PDMFactory.cpp#194

1. Is it intended or a limitation since we cannot get Document object inside the module?

2. Through the code path, I don't see any code invoke the StoreFormatDiagnostics. Therefore, I have no chance to run my code for verification.


3. How to make the ```DispatchNotification``` happen since I want to see the effect of the decoder doctor.

4. The idea of get/set nsGkAtoms::decoderDoctor into document object is to take the advantage of
   a. removing property will trigger the callback so we can easily control when to stop logging.
   b. removing property will also be triggered when document(tab) is closing by user?
   Am I right?


Thank you very much.
Comment on attachment 8797369 [details]
Bug 1297003 - Part1-Store the error information in MediaResult while creating a/v decoders

https://reviewboard.mozilla.org/r/82958/#review81626

r+ after you fixed these, please:

::: dom/locales/en-US/chrome/dom/dom.properties:135
(Diff revision 1)
>  # LOCALIZATION NOTE: %S is a comma-separated list of codecs (e.g. 'video/mp4, video/webm')
>  MediaNoDecoders=No decoders for some of the requested formats: %S
> +# LOCALIZATION NOTE: %S is either 'audio' or 'video'

I think '%S' will be more than just 'audio' or 'video', it should be the same as explained above, i.e.: "a comma-separated list of codecs (e.g. 'video/mp4, video/webm')".

::: dom/media/DecoderDoctorDiagnostics.h:86
(Diff revision 1)
> +  bool DidVideoNotSupported() const { return mVideoNotSupported; }
> +
>    void SetAudioNotSupported() { mAudioNotSupported = true; }
> +  bool DidAudioNotSupported() const { return mAudioNotSupported; }

Method names: 'Did...' -> 'Is...'

::: dom/media/DecoderDoctorDiagnostics.cpp:435
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsAutoString playableFormats;
>    nsAutoString unplayableFormats;
> +  nsAutoString unsupportedMediaFormat;

Please add an 's' to the name, as it's a list where there could be more than one format.

::: dom/media/DecoderDoctorDiagnostics.cpp:607
(Diff revision 1)
> +      if (!unsupportedMediaFormat.IsEmpty()) {
> +        DD_INFO("DecoderDoctorDocumentWatcher[%p, doc=%p]::SynthesizeAnalysis() - Cannot play media, unsupported formats: %s",
> +                this, mDocument, NS_ConvertUTF16toUTF8(unsupportedMediaFormat).get());
> +        ReportAnalysis(mDocument, sMediaFormatNotSupported,
> +                       false, unsupportedMediaFormat);
> +

Remove empty line please.

::: dom/media/DecoderDoctorDiagnostics.cpp:608
(Diff revision 1)
> +        DD_INFO("DecoderDoctorDocumentWatcher[%p, doc=%p]::SynthesizeAnalysis() - Cannot play media, unsupported formats: %s",
> +                this, mDocument, NS_ConvertUTF16toUTF8(unsupportedMediaFormat).get());
> +        ReportAnalysis(mDocument, sMediaFormatNotSupported,
> +                       false, unsupportedMediaFormat);
> +
> +      } else {

I'd prefer if you followed the same pattern as above, i.e.:
  if (test) {
    DD_INFO(...);
    ReportAnalysis(...);
    return;
  }
but no 'else' clause, and leave the last report block unindented (as it's the natural fallback if all other tests above fail).

This way it's easier to add more tests later on.

I've just see your comment #10, I'll respond to it next, bug I think this patch here is still needed anyway.
Attachment #8797369 - Flags: review?(gsquelart) → review+
(In reply to James Cheng[:JamesCheng] from comment #10)
> There is no DecoderDoctorDiagnostics passing to the API
> So it will get the null pointer without any diagnostic effect
> 1. Is it intended or a limitation since we cannot get Document object inside
> the module?
> 
> 2. Through the code path, I don't see any code invoke the
> StoreFormatDiagnostics. Therefore, I have no chance to run my code for
> verification.
> 
> 3. How to make the ```DispatchNotification``` happen since I want to see the
> effect of the decoder doctor.
> 
> 4. The idea of get/set nsGkAtoms::decoderDoctor into document object is to
> take the advantage of
>    a. removing property will trigger the callback so we can easily control
> when to stop logging.
>    b. removing property will also be triggered when document(tab) is closing
> by user?
>    Am I right?

First, to answer your questions:

You're right, we need a DecoderDoctorDiagnostics object to pass around, and eventually call a 'Store...' method on it.
Yes, we require an nsIDocument* to both attach to it (and be destroyed if necessary), and to later forward messages to the console and frontend notification handler.

And because these methods work on the document, they must be called from the main thread. But I see that CreateDecoder is called on a task queue, so this would be "interesting" to solve!

Finding a document is possible: From the MediaFormatReader, the parent class MediaDecoderReader keeps a pointer to the AbstractMediaDecoder, which can give you GetOwner(), which is effectively the HTMLMediaElement, which knows about the document... Easy, right!?

--

Now, thinking about all this, and seeing jya's new 'MediaResult', I'm thinking that maybe we should use that instead.
(Sorry, that would mean scrapping all the DecoderDoctor work you've done so far! Or at least modifying it heavily.)

Roughly speaking:
- Add an 'MediaResult* mError = nullptr;' field to CreateDecoderParams (don't forget to add the corresponding 'Set' method). It feels a bit dangerous, but as far as I can see, CreateDecoderParams are never stored for longer than a function call -- maybe we should make the struct MOZ_STACK_CLASS to prevent non-stack storage.
- Your code from bug 1291742 in PDMFactory::CreateDecoderWithPDM would set *mError (if the pointer is not null) to an appropriate code+message.
- In MediaFormatReader::EnsureDecoderCreated, have a local 'MediaResult result(NS_OK);' and pass its pointer to CreateDecoder, it should go into the CreateDecoderParams. And at the bottom of EnsureDecoderCreated, return that 'result' if not NS_OK anymore.

This error should magically end up in the error object of the video element in the page, which is probably the best logical place for it to be anyway.

Please let me know if that makes sense, and I'm happy to discuss this further before you choose a direction.
Thank you! I should need some time to absorb your word.

I will try to provide new patch soon.
Comment on attachment 8797369 [details]
Bug 1297003 - Part1-Store the error information in MediaResult while creating a/v decoders

Hi Gerald,

Thanks for your guidance,

I think storing the error in the mediaresult is reasonable.

Please see the new patch.

Thank you.
Attachment #8797369 - Flags: review+ → review?(gsquelart)
Comment on attachment 8797369 [details]
Bug 1297003 - Part1-Store the error information in MediaResult while creating a/v decoders

https://reviewboard.mozilla.org/r/82958/#review82364

r? while you're working on the next version...

::: dom/media/MediaFormatReader.cpp:1
(Diff revision 2)
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

In commit description, "MediaRestult" -> "MediaResult".

::: dom/media/platforms/PDMFactory.cpp:238
(Diff revision 2)
>    auto reason = supportChecker.Check();
>    if (reason != SupportChecker::Result::kSupported) {

As discussed, you may want SupportChecker to directly return a MediaResult, this will give use a more precise error message.

::: dom/media/platforms/PDMFactory.cpp:248
(Diff revision 2)
>          diagnostics->SetVideoNotSupported();
> +      }
> +      if (result) {
> +        *result = MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                              RESULT_DETAIL("Decoder may not have the capability to handle the requested video format."
> +                                            "Please check the detail of the media content."));

Add space before 'Please'.
Comment on attachment 8797369 [details]
Bug 1297003 - Part1-Store the error information in MediaResult while creating a/v decoders

https://reviewboard.mozilla.org/r/82958/#review82440

r+ with comments addressed:

::: dom/media/MediaFormatReader.cpp:404
(Diff revision 3)
>  
>    decoder.mDecoderInitialized = false;
>  
>    MonitorAutoLock mon(decoder.mMonitor);
>  
> +  // result may be meaningful by

I don't understand this comment.

::: dom/media/MediaFormatReader.cpp:438
(Diff revision 3)
>      default:
>        break;
>    }
> -  if (decoder.mDecoder ) {
> +  if (decoder.mDecoder) {
>      decoder.mDescription = decoder.mDecoder->GetDescriptionName();
> -    return NS_OK;
> +    result = MediaResult(NS_OK, decoder.mDescription);

I don't think you should add a message to an NS_OK MediaResult, just do 'result = NS_OK' like at line 388.

::: dom/media/platforms/PDMFactory.cpp:96
(Diff revision 3)
> +    explicit CheckResult(Reason aReason,
> +                         MediaResult aResult = MediaResult(NS_OK))
> +      : mReason(aReason),
> +        mMediaResult(mozilla::Move(aResult)) {

CheckResult takes aResult by value, so I believe there will always be a copy here.
You could try and use a forwarding reference for best efficiency.
The default value makes things a bit harder. The best option I can think of is to have two constructors:
  explicit CheckResult(Reason aReason) : mReason(aReason) {}

  template <typename MR>
  CheckResult(Reason aReason, MR aResult)
    : mReason(aReason)
    , mMediaResult(mozilla::Forward<MR>(aResult))
  {}

But this might be a bit too much premature optimization! What do you think?

::: dom/media/platforms/PDMFactory.cpp:99
(Diff revision 3)
> +        mMediaResult(mozilla::Move(aResult)) {
> +
> +    }

Style: A function opening brace should be on a new line.
In the case of an empty function, you can put both on the same line, e.g.:
  CheckResult(...)
    : ...
  {}

::: dom/media/platforms/PDMFactory.cpp:280
(Diff revision 3)
>    if (config.IsAudio()) {
>      m = aPDM->CreateAudioDecoder(aParams);
>      return m.forget();
>    }
>  
>    if (!config.IsVideo()) {
> +    *result = MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                          RESULT_DETAIL("Configuration error."
> +                                        " Intend to create video decoder but the config is not for video."));

I think you end up here because the config doesn't have video or audio, so I think the message should be more like "Decoder configuration error, expected audio or video."
Attachment #8797369 - Flags: review?(gsquelart) → review+
Thanks for the reviewing!
Keywords: checkin-needed
Summary: [Decoder Doctor] Display unsupported a/v format warning in console → Store unsupported a/v format warning in MediaResult.
Comment on attachment 8797369 [details]
Bug 1297003 - Part1-Store the error information in MediaResult while creating a/v decoders

https://reviewboard.mozilla.org/r/82958/#review82712

r+ with comments adressed:

In commit description, "MediaRestult" -> "MediaResult".

::: dom/media/platforms/PDMFactory.cpp:279
(Diff revision 4)
>    if (config.IsAudio()) {
>      m = aPDM->CreateAudioDecoder(aParams);
>      return m.forget();
>    }
>  
>    if (!config.IsVideo()) {
> +    *result = MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                          RESULT_DETAIL("Decoder configuration error, expected video."));

I still think you should mention 'audio' in the error: We're here because both 'config.IsAudio()' and 'config.IsVideo()' were false.
I.e.: "Decoder configuration error, expected audio or video."
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b985aa1c46d3
Part0- Fix nit. r=gerald
https://hg.mozilla.org/integration/autoland/rev/abe848d07d02
Part1-Store the error information in MediaResult while creating a/v decoders r=gerald
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b985aa1c46d3
https://hg.mozilla.org/mozilla-central/rev/abe848d07d02
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: