1377370 - Move HLSResource out of MediaResource::Create()

Status: RESOLVED FIXED

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

Description

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

HLSDecoder should create an HLSResource directly without going through MediaResource::Create() because it is the only resource type that HLSDecoder can handle.

Comment 1

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

Attachment: Bug 1377370. P1 - remove unused HLSResource::mURI.

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

Comment 2

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

Attachment: Bug 1377370. P1 - let HLSDecoder override Load() so it can create an HLSResource directly without going through MediaResource::Create().

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

Comment 3

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

Attachment: Bug 1377370. P2 - simplify HLSDecoder::Clone() for HLSResource doesn't support clone at all.

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

Comment 4

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

Attachment: Bug 1377370. P3 - remove HLS related code from MediaResource.cpp.

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

Comment 5

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

Comment on attachment 8882486 [details]
Bug 1377370. P1 - let HLSDecoder override Load() so it can create an HLSResource directly without going through MediaResource::Create().

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

Comment 6

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

Comment on attachment 8882487 [details]
Bug 1377370. P2 - simplify HLSDecoder::Clone() for HLSResource doesn't support clone at all.

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

Comment 7

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

Comment on attachment 8882488 [details]
Bug 1377370. P3 - remove HLS related code from MediaResource.cpp.

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

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8882487 [details]
Bug 1377370. P2 - simplify HLSDecoder::Clone() for HLSResource doesn't support clone at all.

https://reviewboard.mozilla.org/r/153610/#review158854

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8882488 [details]
Bug 1377370. P3 - remove HLS related code from MediaResource.cpp.

https://reviewboard.mozilla.org/r/153612/#review158856

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8882486 [details]
Bug 1377370. P1 - let HLSDecoder override Load() so it can create an HLSResource directly without going through MediaResource::Create().

https://reviewboard.mozilla.org/r/153608/#review158864

Could you correct the comment in MediaDeocder related to the behavior of Load [1], i.e. 
s/'Returns what was passed to Load()...'/'Returns what was created inside Load()...'/

[1] http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/dom/media/MediaDecoder.h#135

::: dom/media/ChannelMediaDecoder.h:67
(Diff revision 2)
> -  nsresult Load(nsIChannel* aChannel,
> +  virtual nsresult Load(nsIChannel* aChannel,
> -                bool aIsPrivateBrowsing,
> +                        bool aIsPrivateBrowsing,
> -                nsIStreamListener** aStreamListener);
> +                        nsIStreamListener** aStreamListener);
> -  nsresult Load(MediaResource* aOriginal);
> +  virtual nsresult Load(MediaResource* aOriginal);
>  

Do you consider to move |Load| function into base class MediaDecoder, i.e. |ChannelMediaDecoder::Load| and |MediaSourceDecodeR::Load|, then put some comment for it.
I suppose it should be.

Eventually, MediaResource now is created inside |Load| (or passed into |Load| !? [1], this comment should be modified !), I think for developers, it would be more clear to have serverl overloaded |Load| virutal functions in MediaDecoder.  
Or maybe we could have a unitfied |Load| and to elimitate the different code path for MediaSourceDecoder & ChannelMediaDecoder in HTLMMediaElement.




[1] http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/dom/media/MediaDecoder.h#134-145

Comment 11

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

Comment on attachment 8882486 [details]
Bug 1377370. P1 - let HLSDecoder override Load() so it can create an HLSResource directly without going through MediaResource::Create().

https://reviewboard.mozilla.org/r/153608/#review158864

> Do you consider to move |Load| function into base class MediaDecoder, i.e. |ChannelMediaDecoder::Load| and |MediaSourceDecodeR::Load|, then put some comment for it.
> I suppose it should be.
> 
> Eventually, MediaResource now is created inside |Load| (or passed into |Load| !? [1], this comment should be modified !), I think for developers, it would be more clear to have serverl overloaded |Load| virutal functions in MediaDecoder.  
> Or maybe we could have a unitfied |Load| and to elimitate the different code path for MediaSourceDecoder & ChannelMediaDecoder in HTLMMediaElement.
> 
> 
> 
> 
> [1] http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/dom/media/MediaDecoder.h#134-145

No, I am doing the opposite. See bug 1376227 for the reason.

Comment 12

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

Comment on attachment 8882486 [details]
Bug 1377370. P1 - let HLSDecoder override Load() so it can create an HLSResource directly without going through MediaResource::Create().

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

Comment 13

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

Comment on attachment 8882487 [details]
Bug 1377370. P2 - simplify HLSDecoder::Clone() for HLSResource doesn't support clone at all.

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

Comment 14

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

Comment on attachment 8882488 [details]
Bug 1377370. P3 - remove HLS related code from MediaResource.cpp.

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

Comment 15

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

Thanks for the review!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e424f8c227f3
P1 - let HLSDecoder override Load() so it can create an HLSResource directly without going through MediaResource::Create(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/82882f9ca1fd
P2 - simplify HLSDecoder::Clone() for HLSResource doesn't support clone at all. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/768db949de27
P3 - remove HLS related code from MediaResource.cpp. r=kikuo

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e424f8c227f3
https://hg.mozilla.org/mozilla-central/rev/82882f9ca1fd
https://hg.mozilla.org/mozilla-central/rev/768db949de27