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

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1331862. Part 1 - extract code to functions for reuse.

Review commit: https://reviewboard.mozilla.org/r/105952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105952/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1331862. Part 2 - add functions to collect debugging info asynchronously.

Review commit: https://reviewboard.mozilla.org/r/105954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105954/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1331862. Part 3 - remove MDSM::DumpDebugInfo() and call RequestDebugInfo() instead.

Review commit: https://reviewboard.mozilla.org/r/105956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105956/

Comment 4

[Tzuhao Kuo [: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...

Comment 5

[Tzuhao Kuo [:kaku]]

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 6

[Tzuhao Kuo [:kaku]]

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?

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

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 8

[JW Wang [:jwwang] [:jw_wang]]

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 9

[JW Wang [:jwwang] [:jw_wang]]

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 10

[Tzuhao Kuo [:kaku]]

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

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

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0d1993fede0c
https://hg.mozilla.org/mozilla-central/rev/3ad73522af3f
https://hg.mozilla.org/mozilla-central/rev/115d0824834d