1350250 - [Fennec][HLS] HLSResource should obtain buffer information from GeckoHlsPlayer.java

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, enhancement)

Description

[Kilik Kuo [:kikuo] (inactive)]

A JNI Wrapper should be provided for HLSResource to obtain buffer information, i.e. buffered length, buffering status, isEnded, isExpectingMoreData...etc.

These information should be queried from our GeckoHlsPlayer singleton instance.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

HLSResource native code will query the information by JNI from Java implementation trough
this wrapper class.

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

Comment 2

[James Cheng[:JamesCheng]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136392/diff/1-2/

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

NOTE: The call sites for this java wrapper will be provided in Bug 1350246.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

https://reviewboard.mozilla.org/r/136392/#review139912

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsResourceWrapper.java:13
(Diff revision 2)
> +
> +import org.mozilla.gecko.annotation.WrapForJNI;
> +import org.mozilla.gecko.mozglue.JNIObject;
> +
> +public class GeckoHlsResourceWrapper {
> +    private static final String LOGTAG = "GeckoHlsResourceWrapper";

name of member variable should be prefixed.

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

https://reviewboard.mozilla.org/r/136392/#review141512

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsResourceWrapper.java:18
(Diff revision 2)
> +    private static final String LOGTAG = "GeckoHlsResourceWrapper";
> +    private static final boolean DEBUG = false;
> +    private GeckoHlsPlayer player = null;
> +    private boolean destroyed = false;
> +
> +    public static class HlsResourceCallbacks extends JNIObject

Please be sure native callbacks won't use native objects after free.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsResourceWrapper.java:50
(Diff revision 2)
> +    }
> +
> +    @WrapForJNI(calledFrom = "gecko")
> +    public static GeckoHlsResourceWrapper create(String url,
> +                                                 GeckoHlsPlayer.ResourceCallbacks callback) {
> +        GeckoHlsResourceWrapper wrapper = new GeckoHlsResourceWrapper(url, callback);

`return new ...`

Comment 6

[John Lin [:jhlin][:jolin]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

https://reviewboard.mozilla.org/r/136392/#review141938

Comment 7

[James Cheng[:JamesCheng]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136392/diff/2-3/

Comment 8

[James Cheng[:JamesCheng]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

https://reviewboard.mozilla.org/r/136392/#review139912

> name of member variable should be prefixed.

rename the variable with "m" prefix.

Comment 9

[James Cheng[:JamesCheng]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

https://reviewboard.mozilla.org/r/136392/#review141512

> Please be sure native callbacks won't use native objects after free.

thanks for reminding, we will make sure the callback never be invoked if the native object is being free.

Comment 10

[James Cheng[:JamesCheng]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

https://reviewboard.mozilla.org/r/136392/#review141512

> `return new ...`

fixed

Comment 11

[James Cheng[:JamesCheng]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136392/diff/3-4/

Comment 12

[James Cheng[:JamesCheng]]

Comment on attachment 8864736 [details]
Bug 1350250 - Add a GeckoHlsResourceWrapper java glue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136392/diff/4-5/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55a1612e3e58
Add a GeckoHlsResourceWrapper java glue. r=jolin,jya

Comment 14

[James Cheng[:JamesCheng]]

we may create a bug to solve Bug 1368925 by make @WrapForJNI java code independent.

just land this code and solve it in another bug.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55a1612e3e58

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This was reverted for Beta 55 as part of bug 1368925. It remains enabled on Trunk, which is now tracking Gecko 56.