Closed Bug 1331289 Opened 3 years ago Closed 3 years ago

Use MediaContainerType in MediaResource and dependencies

Categories

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

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Continue sprinkling MediaContentType where MIME strings are passed.

This session will start with MediaResource. But as it is used in many places, I'm afraid the patch will have to touch a lot of files at once.
Comment on attachment 8826995 [details]
Bug 1331289 - Use MediaContainerType in MediaResource, SourceBuffer, TrackBuffersManager, and dependencies -

Retracting r? for now, as I'd like to do something else before this bug, which will mean reworking this patch...
Attachment #8826995 - Flags: review?(jyavenard)
Depends on: 1331770
Comment on attachment 8826995 [details]
Bug 1331289 - Use MediaContainerType in MediaResource, SourceBuffer, TrackBuffersManager, and dependencies -

https://reviewboard.mozilla.org/r/104836/#review106230

::: dom/media/Benchmark.cpp:49
(Diff revision 2)
>      sHasRunTest = true;
>  
>      RefPtr<WebMDemuxer> demuxer =
> -      new WebMDemuxer(new BufferMediaResource(sWebMSample, sizeof(sWebMSample), nullptr,
> -                                              NS_LITERAL_CSTRING("video/webm")));
> +      new WebMDemuxer(
> +        new BufferMediaResource(sWebMSample, sizeof(sWebMSample), nullptr,
> +                                MediaContainerType(MEDIAMIMETYPE("video/webm"))));

couldn't we have a macro that is the equivalent of MediaContainerType(MEDIAMIMETYPE(...)) ?

::: dom/media/MediaResource.cpp:1501
(Diff revision 2)
>    NS_ENSURE_SUCCESS(rv, nullptr);
>  
> -  nsAutoCString contentType;
> -  aChannel->GetContentType(contentType);
> +  nsAutoCString contentTypeString;
> +  aChannel->GetContentType(contentTypeString);
> +  Maybe<MediaContainerType> containerType = MakeMediaContainerType(contentTypeString);
> +  if (!containerType) {

do we handle this case of returning nullptr though?

I doubt it, so maybe a strong assert would be preferable for now

::: dom/media/mediasource/TrackBuffersManager.cpp:814
(Diff revision 2)
>  TrackBuffersManager::CreateDemuxerforMIMEType()
>  {
>    ShutdownDemuxers();
>  
> -  if (mType.LowerCaseEqualsLiteral("video/webm") ||
> -      mType.LowerCaseEqualsLiteral("audio/webm")) {
> +  if (mType.Type() == MEDIAMIMETYPE("video/webm")
> +      || mType.Type() == MEDIAMIMETYPE("audio/webm")) {

while the || at the beginning of the next line, this is not the style used across the file.

So we now have non-consistent style.

You can fix the styling in a separate patch

::: dom/media/mediasource/gtest/TestContainerParser.cpp:43
(Diff revision 2)
>    return buffer.forget();
>  }
>  
>  TEST(ContainerParser, ADTSHeader) {
>    nsAutoPtr<ContainerParser> parser;
> -  parser = ContainerParser::CreateForMIMEType(NS_LITERAL_CSTRING("audio/aac"));
> +  parser = ContainerParser::CreateForMIMEType(MediaContainerType(MEDIAMIMETYPE("audio/aac")));

80 columns

::: dom/media/mediasource/gtest/TestContainerParser.cpp:105
(Diff revision 2)
>    EXPECT_EQ(parser->MediaSegmentRange(), MediaByteRange());
>  }
>  
>  TEST(ContainerParser, ADTSBlankMedia) {
>    nsAutoPtr<ContainerParser> parser;
> -  parser = ContainerParser::CreateForMIMEType(NS_LITERAL_CSTRING("audio/aac"));
> +  parser = ContainerParser::CreateForMIMEType(MediaContainerType(MEDIAMIMETYPE("audio/aac")));

80 columns
Attachment #8826995 - Flags: review?(jyavenard) → review+
Comment on attachment 8826995 [details]
Bug 1331289 - Use MediaContainerType in MediaResource, SourceBuffer, TrackBuffersManager, and dependencies -

https://reviewboard.mozilla.org/r/104836/#review106230

> couldn't we have a macro that is the equivalent of MediaContainerType(MEDIAMIMETYPE(...)) ?

We could, but is it appropriate? :-)
I'll open a separate bug for that...

> do we handle this case of returning nullptr though?
> 
> I doubt it, so maybe a strong assert would be preferable for now

A few lines above, there's an `NS_ENSURE_SUCCESS(rv, nullptr);` that's been there for more than 4 years!
And AFAICS its only use is from HTMLMediaElement::InitializeDecoderForChannel, where the returned value is checked for nullptr.

> while the || at the beginning of the next line, this is not the style used across the file.
> 
> So we now have non-consistent style.
> 
> You can fix the styling in a separate patch

I despise these end-of-line operators! But agreed that I should follow the file's style; I usually do, I just didn't see another one in the same screenful.
'MediaContainerType' is the new 'MediaContentType'.
Summary: Use MediaContentType in MediaResource and dependencies → Use MediaContainerType in MediaResource and dependencies
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/922366c67e72
Use MediaContainerType in MediaResource, SourceBuffer, TrackBuffersManager, and dependencies - r=jya
Blocks: 1331940
https://hg.mozilla.org/mozilla-central/rev/922366c67e72
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.