New classes MediaMIMEType&friends to separate pure-data handling from context of MediaContentType
RESOLVED
FIXED
in Firefox 53
Status
()
People
(Reporter: gerald, Assigned: gerald)
Tracking
(Blocks 1 bug)
Firefox Tracking Flags
(firefox53 fixed)
Details
Attachments
(14 attachments)
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
MediaContentType does all the work of handling MIME type strings in the context of content types. We want to separate the context (content types, what the data means to represent) from the data-handling (MIME strings, what the data actually is and how to manipulate its components in the safest way) for easier maintainability, and also because the same data-handling could potentially be used in different contexts. To see the proposed patches in a broader context: https://hg.mozilla.org/try/pushloghtml?changeset=2743aefd9eede9f207238ba71a48ccb5382af848
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
Assignee | |
Comment 14•3 years ago
|
||
Jean-Yves: As per the description, there are more bugs&patches to come after these, where I actually use the new types around dom/media, starting in DecoderTraits and its immediate dependencies, before going further. Please have a look there if you'd like to see some example of these uses, that I hope will justify my design choices here: https://hg.mozilla.org/try/pushloghtml?changeset=2743aefd9eede9f207238ba71a48ccb5382af848 But I will first let you review bug 1329561, bug 1329564, and this bug 1329568, before I throw more code at you! Especially as your review comments here could lead to changes in my proposed later bugs&patches.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 25•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824880 [details] Bug 1329568 - Add missing #include in MediaStreamListener.h - https://reviewboard.mozilla.org/r/103200/#review103834
Attachment #8824880 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 26•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824881 [details] Bug 1329568 - MediaExtendedMIMEType - https://reviewboard.mozilla.org/r/103202/#review103836
Attachment #8824881 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 27•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824882 [details] Bug 1329568 - MediaMIMEType - https://reviewboard.mozilla.org/r/103204/#review103838 ::: dom/media/MediaContentType.h:22 (Diff revision 1) > class MediaContentType > { > public: > + explicit MediaContentType(const MediaMIMEType& aType) > + : mExtendedMIMEType(aType) > + {} this is a bit inconsistent with the rest of the code that use: { } on two lines ::: dom/media/MediaContentType.h:23 (Diff revision 1) > { > public: > + explicit MediaContentType(const MediaMIMEType& aType) > + : mExtendedMIMEType(aType) > + {} > + explicit MediaContentType(MediaMIMEType&& aType) i don't think we need explicit for move constructor ::: dom/media/MediaMIMETypes.h:73 (Diff revision 1) > } > > - nsCString mMIMEType; // UTF8 MIME type. > + MediaMIMEType mMIMEType; // MIME type/subtype. > bool mHaveCodecs; // If false, mCodecs must be empty. > nsString mCodecs; > int32_t mWidth; // -1 if not provided. please use C++11 initialization as mentioned below. ::: dom/media/MediaMIMETypes.cpp:85 (Diff revision 1) > return number; > } > > +MediaExtendedMIMEType::MediaExtendedMIMEType(const MediaMIMEType& aType) > + : mMIMEType(aType) > + , mHaveCodecs(false) C++11 initalization would be better
Attachment #8824882 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 28•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824883 [details] Bug 1329568 - Simple IsMediaMIMEType checker for strings - https://reviewboard.mozilla.org/r/103206/#review103840
Attachment #8824883 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 29•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824884 [details] Bug 1329568 - Restrict MediaMIMEType to IsMediaMIMEType-checked strings - https://reviewboard.mozilla.org/r/103208/#review103842 ::: dom/media/MediaMIMETypes.cpp:26 (Diff revision 2) > if (!NS_SUCCEEDED(rv) || mime.IsEmpty()) { > return false; > } > > - mMIMEType = NS_ConvertUTF16toUTF8(mime); > + NS_ConvertUTF16toUTF8 mime8(mime); > + if (!IsMediaMIMEType(mime8.Data(), mime8.Length())) { couldn't we have a function taking the UTF8 string directly ?
Attachment #8824884 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 30•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824885 [details] Bug 1329568 - MediaMIMEType construction from literal string - https://reviewboard.mozilla.org/r/103210/#review103844 nice
Attachment #8824885 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 31•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824886 [details] Bug 1329568 - MediaMIMEType comparisons against others - https://reviewboard.mozilla.org/r/103212/#review103846
Attachment #8824886 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 32•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824887 [details] Bug 1329568 - MediaMIMEType 'audio/' and 'video/' checks - https://reviewboard.mozilla.org/r/103214/#review103848 ::: dom/media/MediaMIMETypes.h:79 (Diff revision 2) > bool operator!=(const MediaMIMEType& aOther) const > { > return !mMIMEType.Equals(aOther.mMIMEType); > } > > + bool IsAudio() const; One has to be careful that video/blah is a valid mimetype for a container with only an audio track. e.g. video/webm can contain a combination of one or more of the following: video/vp9 video/vp8 audio/opus audio/vorbis
Attachment #8824887 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 33•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824888 [details] Bug 1329568 - MediaCodecs - https://reviewboard.mozilla.org/r/103216/#review103850 ::: dom/media/MediaMIMETypes.cpp:177 (Diff revision 2) > return false; > } > > mMIMEType.SetType(nsCString(mime8)); > > - rv = parser.GetParameter("codecs", mCodecs); > + nsAutoString codecs; Unused variable?
Attachment #8824888 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 34•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824889 [details] Bug 1329568 - Remove MediaContentType crutches - https://reviewboard.mozilla.org/r/103218/#review103852
Attachment #8824889 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 35•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824890 [details] Bug 1329568 - Media...Type::SizeOf... methods - https://reviewboard.mozilla.org/r/103220/#review103854 LGTM, want to run that passed njn...
Attachment #8824890 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 36•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824891 [details] Bug 1329568 - Store original media-content string - https://reviewboard.mozilla.org/r/103222/#review103856 ::: dom/media/MediaContentType.h:38 (Diff revision 2) > const MediaMIMEType& Type() const { return mExtendedMIMEType.Type(); } > const MediaExtendedMIMEType& ExtendedType() const { return mExtendedMIMEType; } > > + // Original string. Note that "type/subtype" may not be lowercase, > + // use Type().AsString() instead. > + const nsACString& AsString() const { return mExtendedMIMEType.AsString(); } I think we should make the name a bit more explicit that it's the original string. AsOriginalString maybe?
Attachment #8824891 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 37•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824892 [details] Bug 1329568 - gtest for MediaMIMEType and family - https://reviewboard.mozilla.org/r/103224/#review103858
Attachment #8824892 -
Flags: review?(jyavenard) → review+
|
|
||
Updated•3 years ago
|
Attachment #8824890 -
Flags: review?(n.nethercote)
|
||
Comment 38•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8824890 [details] Bug 1329568 - Media...Type::SizeOf... methods - https://reviewboard.mozilla.org/r/103220/#review103990 Thank you for asking me to check this. Looks good except for one small thing. ::: dom/media/MediaContentType.cpp:16 (Diff revision 2) > namespace mozilla { > > +size_t > +MediaContentType::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const > +{ > + return mExtendedMIMEType.SizeOfIncludingThis(aMallocSizeOf); This should call SizeOfExcludingThis() because mExtendedMIMEType is an inline member, not a pointer. (This mistake can lead to crashes, because aMallocSizeOf will be called on an invalid pointer. Have you tested this code?) (BTW, https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting is the docs for writing memory reporters, if you haven't seen it before.)
Attachment #8824890 -
Flags: review?(n.nethercote) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 65•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8824882 [details] Bug 1329568 - MediaMIMEType - https://reviewboard.mozilla.org/r/103204/#review103838 > this is a bit inconsistent with the rest of the code that use: > { > } > on two lines I guess I was trying to save vertical space in a header. But I suppose consistency is best. I'll fix these in other files, if any. > i don't think we need explicit for move constructor It's a converting constructor (the argument is of a different type from the class), so we definitely need an explicit here. > please use C++11 initialization as mentioned below. You could have told me in bug 1329561, or even in the previous patch! :-) I intentionally used old-school initialization through constructors only, as there is an empty constructor that doesn't do anything followed by Populate() to actually initialize the values. But I have found a way to instead use a fully-argumented constructor instead, which I introduce in the previous patch ("MediaExtendedMIMEType"). And in this patch here I introduce member in-line initialization, which is now needed as there are new constructors that don't need to give custum values to all members.
|
|
Assignee | |
Comment 66•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8824884 [details] Bug 1329568 - Restrict MediaMIMEType to IsMediaMIMEType-checked strings - https://reviewboard.mozilla.org/r/103208/#review103842 > couldn't we have a function taking the UTF8 string directly ? Sure, easy enough.
|
|
Assignee | |
Comment 67•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8824887 [details] Bug 1329568 - MediaMIMEType 'audio/' and 'video/' checks - https://reviewboard.mozilla.org/r/103214/#review103848 > One has to be careful that video/blah is a valid mimetype for a container with only an audio track. > > e.g. > video/webm > can contain a combination of one or more of the following: > video/vp9 video/vp8 audio/opus audio/vorbis These only test the major type, i.e., whether the string starts with "audio/" or "video/". But I see that the name could give the wrong impression that they will definitely classify a type as audio or video, which could be incorrect as you demonstrate. So I will rename them to "Has{Audio,Video}MajorType", and add comments. While at it, I will also add HasApplicationMajorType, as it is an accepted type for some formats.
|
|
Assignee | |
Comment 68•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8824888 [details] Bug 1329568 - MediaCodecs - https://reviewboard.mozilla.org/r/103216/#review103850 > Unused variable? Well-spotted! In the work needed to add in-line member initializers, I've already fixed it -- it's still there, but it's actually used.
|
|
Assignee | |
Comment 69•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8824890 [details] Bug 1329568 - Media...Type::SizeOf... methods - https://reviewboard.mozilla.org/r/103220/#review103990 > This should call SizeOfExcludingThis() because mExtendedMIMEType is an inline member, not a pointer. > > (This mistake can lead to crashes, because aMallocSizeOf will be called on an invalid pointer. Have you tested this code?) > > (BTW, https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting is the docs for writing memory reporters, if you haven't seen it before.) Thank you for finding this issue. Also, as discussed on IRC, I will remove the unneeded 'Including' functions, as the size is only needed when these objects are directly included in others.
|
|
Assignee | |
Comment 70•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8824891 [details] Bug 1329568 - Store original media-content string - https://reviewboard.mozilla.org/r/103222/#review103856 > I think we should make the name a bit more explicit that it's the original string. > > AsOriginalString maybe? Good idea. 'OriginalString()' should do, as I think of it as a different beast -- giving an unmodified copy of some old data -- compared to e.g.: Type().AsString(), which gives us current data converted to a different type.
|
|
Assignee | |
Comment 71•3 years ago
|
||
I had forgotten to publish my review responses, sorry. I've already requested autoland, so if you see anything that's still a concern, I'll open a follow-up bug.
|
||
Comment 72•3 years ago
|
||
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e1ed12f057c8 Add missing #include in MediaStreamListener.h - r=jya https://hg.mozilla.org/integration/autoland/rev/644f206d06d6 MediaExtendedMIMEType - r=jya https://hg.mozilla.org/integration/autoland/rev/2ce9dcf0c274 MediaMIMEType - r=jya https://hg.mozilla.org/integration/autoland/rev/99758b5f7918 Simple IsMediaMIMEType checker for strings - r=jya https://hg.mozilla.org/integration/autoland/rev/7c62078beb88 Restrict MediaMIMEType to IsMediaMIMEType-checked strings - r=jya https://hg.mozilla.org/integration/autoland/rev/662529436cc4 MediaMIMEType construction from literal string - r=jya https://hg.mozilla.org/integration/autoland/rev/453851b40e3e MediaMIMEType comparisons against others - r=jya https://hg.mozilla.org/integration/autoland/rev/3a49367220e3 MediaMIMEType 'audio/' and 'video/' checks - r=jya https://hg.mozilla.org/integration/autoland/rev/da855d6a78bb MediaCodecs - r=jya https://hg.mozilla.org/integration/autoland/rev/90a091e65db3 Remove MediaContentType crutches - r=jya https://hg.mozilla.org/integration/autoland/rev/c316c8d24d32 Media...Type::SizeOf... methods - r=jya,njn https://hg.mozilla.org/integration/autoland/rev/706da9f85272 Store original media-content string - r=jya https://hg.mozilla.org/integration/autoland/rev/8f37ee96bd78 gtest for MediaMIMEType and family - r=jya
|
||
Comment 73•3 years ago
|
||
All backed out for wpt failures like https://treeherder.mozilla.org/logviewer.html#?job_id=68580628&repo=autoland https://hg.mozilla.org/integration/autoland/rev/4caa61eee820232d92d427a6077e56bb6eee21b8
Flags: needinfo?(gsquelart)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 88•3 years ago
|
||
The wpt failures came from bug 1176218, but were not apparent until now as I've added extra checks that can actually reject invalid&unexpected media types. A new patch corrects this issue before the new checks are introduced.
Blocks: 1176218
Flags: needinfo?(gsquelart)
|
|
Assignee | |
Comment 89•3 years ago
|
||
Try with these patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fafcbcc01a8b7a5ea35f3dcc2fce3c6b4f281ac Try with these patches plus the following bug 1330284: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89dc1706f922b20a77f62690ac3bac4c0734ace1
|
|
||
Comment 90•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8826480 [details] Bug 1329568 - MediaSource::IsTypeSupported should report a NotSupportedError if type is not supported - https://reviewboard.mozilla.org/r/104442/#review105368 WHO R+ THAT ESRLIER?????
Attachment #8826480 -
Flags: review?(jyavenard) → review+
|
|
||
Comment 91•3 years ago
|
||
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/47e7a628e4eb Add missing #include in MediaStreamListener.h - r=jya https://hg.mozilla.org/integration/autoland/rev/9a83e282245d MediaExtendedMIMEType - r=jya https://hg.mozilla.org/integration/autoland/rev/c38a9125908a MediaMIMEType - r=jya https://hg.mozilla.org/integration/autoland/rev/efa49d608777 MediaSource::IsTypeSupported should report a NotSupportedError if type is not supported - r=jya https://hg.mozilla.org/integration/autoland/rev/170324f26108 Simple IsMediaMIMEType checker for strings - r=jya https://hg.mozilla.org/integration/autoland/rev/09e268ea2617 Restrict MediaMIMEType to IsMediaMIMEType-checked strings - r=jya https://hg.mozilla.org/integration/autoland/rev/ad28a64c910d MediaMIMEType construction from literal string - r=jya https://hg.mozilla.org/integration/autoland/rev/db6199820ea5 MediaMIMEType comparisons against others - r=jya https://hg.mozilla.org/integration/autoland/rev/99bea6f5f7cb MediaMIMEType 'audio/' and 'video/' checks - r=jya https://hg.mozilla.org/integration/autoland/rev/24d6f93f708b MediaCodecs - r=jya https://hg.mozilla.org/integration/autoland/rev/2263f931b739 Remove MediaContentType crutches - r=jya https://hg.mozilla.org/integration/autoland/rev/a5149be026c3 Media...Type::SizeOf... methods - r=jya,njn https://hg.mozilla.org/integration/autoland/rev/c1bcd1c8c386 Store original media-content string - r=jya https://hg.mozilla.org/integration/autoland/rev/dc460f55e042 gtest for MediaMIMEType and family - r=jya
|
||
Comment 92•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/47e7a628e4eb https://hg.mozilla.org/mozilla-central/rev/9a83e282245d https://hg.mozilla.org/mozilla-central/rev/c38a9125908a https://hg.mozilla.org/mozilla-central/rev/efa49d608777 https://hg.mozilla.org/mozilla-central/rev/170324f26108 https://hg.mozilla.org/mozilla-central/rev/09e268ea2617 https://hg.mozilla.org/mozilla-central/rev/ad28a64c910d https://hg.mozilla.org/mozilla-central/rev/db6199820ea5 https://hg.mozilla.org/mozilla-central/rev/99bea6f5f7cb https://hg.mozilla.org/mozilla-central/rev/24d6f93f708b https://hg.mozilla.org/mozilla-central/rev/2263f931b739 https://hg.mozilla.org/mozilla-central/rev/a5149be026c3 https://hg.mozilla.org/mozilla-central/rev/c1bcd1c8c386 https://hg.mozilla.org/mozilla-central/rev/dc460f55e042
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
|
||
Comment 93•3 years ago
|
||
for informational purposes only (no action needed), this set of patches showed a small increase (+1) in the number of constructors: == Change summary for alert #4804 (as of January 13 2017 22:23 UTC) == Regressions: 1% compiler_metrics num_constructors linux32 debug 181 -> 182 1% compiler_metrics num_constructors linux64 debug 181 -> 182 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4804
You need to log in
before you can comment on or make changes to this bug.
Description
•