Closed Bug 1377370 Opened 5 years ago Closed 5 years ago

Move HLSResource out of MediaResource::Create()

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files, 1 obsolete file)

HLSDecoder should create an HLSResource directly without going through MediaResource::Create() because it is the only resource type that HLSDecoder can handle.
Assignee: nobody → jwwang
Blocks: 1373160
Priority: -- → P3
Attachment #8882485 - Attachment is obsolete: true
Attachment #8882486 - Flags: review?(kikuo)
Attachment #8882487 - Flags: review?(kikuo)
Attachment #8882488 - Flags: review?(kikuo)
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
Attachment #8882487 - Flags: review?(kikuo) → review+
Comment on attachment 8882488 [details]
Bug 1377370. P3 - remove HLS related code from MediaResource.cpp.

https://reviewboard.mozilla.org/r/153612/#review158856
Attachment #8882488 - Flags: review?(kikuo) → review+
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
Attachment #8882486 - Flags: review?(kikuo) → review+
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.
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.