Closed
Bug 1331289
Opened 7 years ago
Closed 7 years ago
Use MediaContainerType in MediaResource and dependencies
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
'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
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/922366c67e72
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•