Closed
Bug 1065111
Opened 10 years ago
Closed 10 years ago
Use CFSTR in AppleDecoderModule
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: rillian, Assigned: rillian)
References
Details
Attachments
(5 files, 1 obsolete file)
1.26 KB,
patch
|
jya
:
review-
|
Details | Diff | Splinter Review |
3.06 KB,
patch
|
jya
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
862 bytes,
patch
|
jya
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
jya
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
730 bytes,
patch
|
jya
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
When I wrote AppleVTDecoder I thought the CFSTR() macro, so popular in example code, must be allocating, so I avoided using it. It uses a compiler extension to avoid allocating or running an initializer on MacOS X, so it's actually fine to use for this code. We should go over the implementation and see if there are any places we can improve things by using CFSTR() instead of creating a dynamic CFStringRef.
Assignee | ||
Comment 1•10 years ago
|
||
Supporting investigation at https://stackoverflow.com/questions/25752703/does-cfstr-allocate-memory
Assignee: nobody → giles
Depends on: 1062654
Assignee | ||
Comment 2•10 years ago
|
||
You seem to have removed all the CFString invocations, but I found some obsolete code we can remove.
Attachment #8529932 -
Flags: review?(jyavenard)
Assignee | ||
Updated•10 years ago
|
Attachment #8529932 -
Attachment description: 0002-Bug-1065111-Remove-obsolete-CFString-includes.-r-jya.patch → Part 1: remove obsolete CFString includes.
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8529933 -
Flags: review?(jyavenard)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8529934 -
Flags: review?(jyavenard)
Assignee | ||
Comment 5•10 years ago
|
||
Move the logging getter implementation to AppleDecoderModule.cpp we can remove AppleUtils.cpp entirely and just leave the header.
Attachment #8529937 -
Flags: review?(jyavenard)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8529938 -
Flags: review?(jyavenard)
Comment 7•10 years ago
|
||
Comment on attachment 8529932 [details] [diff] [review] Part 1: remove obsolete CFString includes. Review of attachment 8529932 [details] [diff] [review]: ----------------------------------------------------------------- The CFSTR macro itself is defined in <CoreFoundation/CFString.h> so it can't be removed files that make use of it.
Attachment #8529932 -
Flags: review?(jyavenard) → review-
Updated•10 years ago
|
Attachment #8529933 -
Flags: review?(jyavenard) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8529934 [details] [diff] [review] Part 3: Remove obsolete logging macros. Review of attachment 8529934 [details] [diff] [review]: ----------------------------------------------------------------- shouldn't we have removed the entire AppleUtils.cpp at once? it serves no purpose now
Attachment #8529934 -
Flags: review?(jyavenard) → review+
Updated•10 years ago
|
Attachment #8529937 -
Flags: review?(jyavenard) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8529938 [details] [diff] [review] Part 5: Remove more includes. Review of attachment 8529938 [details] [diff] [review]: ----------------------------------------------------------------- this is the same as part4 by the look of things. that patch wouldn't apply once part4 is applied.
Attachment #8529938 -
Flags: review?(jyavenard) → review-
Assignee | ||
Comment 10•10 years ago
|
||
Sorry about that. This should be the correct patch.
Attachment #8529938 -
Attachment is obsolete: true
Attachment #8529962 -
Flags: review?(jyavenard)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #7) > The CFSTR macro itself is defined in <CoreFoundation/CFString.h> so it can't > be removed files that make use of it. Good point. It still compiles without it. VideoToolbox.h, etc. must end up pulling them in. I can leave it if you like, but we use a bunch of other CF things without explicitly including their headers. CFDictionary, CFType, kCFAllocatorDefault.
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #8) > shouldn't we have removed the entire AppleUtils.cpp at once? I get there in Part 4. I like to split patches up into little pieces. I can fold them together before landing if you prefer.
Comment 13•10 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #11) > (In reply to Jean-Yves Avenard [:jya] from comment #7) > > > The CFSTR macro itself is defined in <CoreFoundation/CFString.h> so it can't > > be removed files that make use of it. > > Good point. It still compiles without it. VideoToolbox.h, etc. must end up > pulling them in. I can leave it if you like, but we use a bunch of other CF > things without explicitly including their headers. CFDictionary, CFType, > kCFAllocatorDefault. yes, it's likely it's included seeing that VideoToolbox takes dictionary with strings as keys... I guess I will wait to see a try run with those... a bit cautious with mac header lately seeing that my stuff on the AAC decoder compiled on my machine, but not the 10.8 try machines.
Updated•10 years ago
|
Attachment #8529962 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #13) > I guess I will wait to see a try run with those... a bit cautious with mac > header lately seeing that my stuff on the AAC decoder compiled on my > machine, but not the 10.8 try machines. Ok, I'll push all 5 to try once the trees reopen. Thanks for the quick review!
Assignee | ||
Comment 15•10 years ago
|
||
Pushed all five patches to try. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=98dc24e3c1a2
Assignee | ||
Comment 16•10 years ago
|
||
Built on try, fwiw. Shall I land with or without the CFString.h removal patch?
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 17•10 years ago
|
||
I also verified locally that the non-unified build still works.
Comment 18•10 years ago
|
||
Personally I prefer to include the headers I'll directly use. That way you don't rely on a obscured code including the stuff you need. But seeing we don't include CFArray and CFDictionary headers either, I guess it's fine not to include them
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 19•10 years ago
|
||
I'll leave the CFString.h includes in there for now. https://hg.mozilla.org/integration/mozilla-inbound/rev/5c94f77346f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/20056b650c74 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2363d723a2e https://hg.mozilla.org/integration/mozilla-inbound/rev/516b0ed28ab4
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5c94f77346f3 https://hg.mozilla.org/mozilla-central/rev/20056b650c74 https://hg.mozilla.org/mozilla-central/rev/d2363d723a2e https://hg.mozilla.org/mozilla-central/rev/516b0ed28ab4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8529933 [details] [diff] [review] Part 2: Remove obsolete CFDict helpers Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: No functional change, but makes backporting other changes simpler. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Low. This just removes dead code. [String/UUID change made/needed]: None
Attachment #8529933 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8529934 [details] [diff] [review] Part 3: Remove obsolete logging macros. Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: No functional change, but makes backporting other changes simpler. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Low. This just removes dead code. [String/UUID change made/needed]: None
Attachment #8529934 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8529937 [details] [diff] [review] Part 4: Remoe AppleUtils.cpp Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: No functional change, but makes backporting other changes simpler. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Low. This just removes dead code. [String/UUID change made/needed]: None
Attachment #8529937 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8529962 [details] [diff] [review] Part 5: Remove more includes. Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: No functional change, but makes backporting other changes simpler. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Low. This just removes dead code. [String/UUID change made/needed]: None
Attachment #8529962 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8529933 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8529934 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8529937 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8529962 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 25•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/19c1fa7c637b https://hg.mozilla.org/releases/mozilla-aurora/rev/40d3a2d82a5e https://hg.mozilla.org/releases/mozilla-aurora/rev/c2e28f540b16 https://hg.mozilla.org/releases/mozilla-aurora/rev/eeaa277587ac
Assignee | ||
Comment 26•9 years ago
|
||
Backed out and relanded to fix commit message corruption. https://hg.mozilla.org/releases/mozilla-aurora/rev/676eee1ff92c https://hg.mozilla.org/releases/mozilla-aurora/rev/b8add81bcccc https://hg.mozilla.org/releases/mozilla-aurora/rev/ee2e7c1e9736 https://hg.mozilla.org/releases/mozilla-aurora/rev/12b7ad6ab23a https://hg.mozilla.org/releases/mozilla-aurora/rev/5d0d448d42bd
You need to log in
before you can comment on or make changes to this bug.
Description
•