Closed Bug 1065111 Opened 10 years ago Closed 9 years ago

Use CFSTR in AppleDecoderModule

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(5 files, 1 obsolete file)

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.
Supporting investigation at https://stackoverflow.com/questions/25752703/does-cfstr-allocate-memory
Assignee: nobody → giles
Depends on: 1062654
You seem to have removed all the CFString invocations, but I found some obsolete code we can remove.
Attachment #8529932 - Flags: review?(jyavenard)
Attachment #8529932 - Attachment description: 0002-Bug-1065111-Remove-obsolete-CFString-includes.-r-jya.patch → Part 1: remove obsolete CFString includes.
Attachment #8529933 - Flags: review?(jyavenard)
Attachment #8529934 - Flags: review?(jyavenard)
Move the logging getter implementation to AppleDecoderModule.cpp we can remove AppleUtils.cpp entirely and just leave the header.
Attachment #8529937 - Flags: review?(jyavenard)
Attached patch Part 5: Remove more includes. (obsolete) — Splinter Review
Attachment #8529938 - Flags: review?(jyavenard)
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-
Attachment #8529933 - Flags: review?(jyavenard) → review+
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+
Attachment #8529937 - Flags: review?(jyavenard) → review+
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-
Sorry about that. This should be the correct patch.
Attachment #8529938 - Attachment is obsolete: true
Attachment #8529962 - Flags: review?(jyavenard)
(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.
(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.
(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.
Attachment #8529962 - Flags: review?(jyavenard) → review+
(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!
Built on try, fwiw. Shall I land with or without the CFString.h removal patch?
Flags: needinfo?(jyavenard)
I also verified locally that the non-unified build still works.
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)
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?
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?
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?
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?
Attachment #8529933 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8529934 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8529937 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8529962 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.