Closed Bug 1331862 Opened 5 years ago Closed 5 years ago

Add a function to retrieve debugging information of decoder/reader asynchronously

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files)

Per bug 1331833 comment 0, we will add a function to collect debugging data from MediaDecoder/MDSM/MFR in an asynchronous way for MDSM members can only be accessed on the task queue thread. The function returns a promise which will be resolved when the collection is done.
Assignee: nobody → jwwang
Blocks: 1331833
Priority: -- → P3
Attachment #8828625 - Flags: review?(kaku)
Attachment #8828626 - Flags: review?(kaku)
Attachment #8828627 - Flags: review?(kaku)
Comment on attachment 8828625 [details]
Bug 1331862. Part 1 - extract code to functions for reuse.

https://reviewboard.mozilla.org/r/105952/#review107382

::: dom/media/MediaDecoder.cpp:1732
(Diff revision 1)
>      ? MediaDecoderOwner::NEXT_FRAME_AVAILABLE
>      : MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE;
>  }
>  
> -void
> -MediaDecoder::DumpDebugInfo()
> +nsCString
> +MediaDecoder::GetDebugInfo()

According to our coding style, Getters that never fail and never return null are named `Foo()`, while all other getters use `GetFoo()`, though, I don't know if we're following this rule tightly or not...
Attachment #8828625 - Flags: review?(kaku) → review+
Comment on attachment 8828626 [details]
Bug 1331862. Part 2 - add functions to collect debugging info asynchronously.

https://reviewboard.mozilla.org/r/105954/#review107406

::: dom/media/MediaDecoder.cpp:1768
(Diff revision 1)
> +  return GetStateMachine()->RequestDebugInfo()->Then(
> +    AbstractThread::MainThread(), __func__,
> +    [str] (const nsACString& aString) {
> +      nsCString result = str + nsCString("\n") + aString;
> +      return DebugInfoPromise::CreateAndResolve(result, __func__);
> +    },
> +    [str] () {
> +      return DebugInfoPromise::CreateAndResolve(str, __func__);

You're returning a promise, which is converted from a ThenCommand, here but who is gonna resolve/reject the returned promise?

I thouhgt the async operations of the ThenCommand should resolve/reject the returned promise?
Comment on attachment 8828627 [details]
Bug 1331862. Part 3 - remove MDSM::DumpDebugInfo() and call RequestDebugInfo() instead.

https://reviewboard.mozilla.org/r/105956/#review107408

::: dom/media/MediaDecoder.cpp:1761
(Diff revision 1)
> +    [this, str] (const nsACString& aString) {
> +      DUMP_LOG("%s", str.get());
> +      DUMP_LOG("%s", aString.Data());
> +    },
> +    [this, str] () {
> +      DUMP_LOG("%s", str.get());
> +    });

I guess that we don't care that the `this` pointer is valid or not here since we're only prointing its value, right?
Attachment #8828627 - Flags: review?(kaku) → review+
Comment on attachment 8828625 [details]
Bug 1331862. Part 1 - extract code to functions for reuse.

https://reviewboard.mozilla.org/r/105952/#review107382

> According to our coding style, Getters that never fail and never return null are named `Foo()`, while all other getters use `GetFoo()`, though, I don't know if we're following this rule tightly or not...

I guess it is not strickly followed. I choose GetDebugInfo because it is not obvious that DebugInfo is a getter since "debug" can be a verb.
Comment on attachment 8828626 [details]
Bug 1331862. Part 2 - add functions to collect debugging info asynchronously.

https://reviewboard.mozilla.org/r/105954/#review107420

::: dom/media/MediaDecoder.cpp:1768
(Diff revision 1)
> +  return GetStateMachine()->RequestDebugInfo()->Then(
> +    AbstractThread::MainThread(), __func__,
> +    [str] (const nsACString& aString) {
> +      nsCString result = str + nsCString("\n") + aString;
> +      return DebugInfoPromise::CreateAndResolve(result, __func__);
> +    },
> +    [str] () {
> +      return DebugInfoPromise::CreateAndResolve(str, __func__);

The promise is a completion promise which will be resolved/rejected when the promise returned by the lambda is resolved/rejected.
Comment on attachment 8828627 [details]
Bug 1331862. Part 3 - remove MDSM::DumpDebugInfo() and call RequestDebugInfo() instead.

https://reviewboard.mozilla.org/r/105956/#review107408

> I guess that we don't care that the `this` pointer is valid or not here since we're only prointing its value, right?

Exactly.
Comment on attachment 8828626 [details]
Bug 1331862. Part 2 - add functions to collect debugging info asynchronously.

https://reviewboard.mozilla.org/r/105954/#review107424
Attachment #8828626 - Flags: review?(kaku) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d1993fede0c
Part 1 - extract code to functions for reuse. r=kaku
https://hg.mozilla.org/integration/autoland/rev/3ad73522af3f
Part 2 - add functions to collect debugging info asynchronously. r=kaku
https://hg.mozilla.org/integration/autoland/rev/115d0824834d
Part 3 - remove MDSM::DumpDebugInfo() and call RequestDebugInfo() instead. r=kaku
You need to log in before you can comment on or make changes to this bug.