WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
52467
REGRESSION (
r72119
): Audio never plays on Star Wars intro animation
https://bugs.webkit.org/show_bug.cgi?id=52467
Summary
REGRESSION (r72119): Audio never plays on Star Wars intro animation
Adam Roben (:aroben)
Reported
2011-01-14 12:19:13 PST
To reproduce: 1. Go to
http://www.gesteves.com/experiments/starwars.html
The audio never plays. It worked just great in 5.0.3.
Attachments
Patch
(2.40 KB, patch)
2011-01-14 23:35 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(2.03 KB, patch)
2011-01-17 11:29 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(151.21 KB, patch)
2011-01-21 11:21 PST
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(184.49 KB, patch)
2011-01-21 17:05 PST
,
Jer Noble
eric.carlson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Adam Roben (:aroben)
Comment 1
2011-01-14 12:21:42 PST
<
rdar://problem/8866928
>
Jer Noble
Comment 2
2011-01-14 23:35:20 PST
Created
attachment 79056
[details]
Patch
Adam Roben (:aroben)
Comment 3
2011-01-17 04:21:20 PST
Comment on
attachment 79056
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=79056&action=review
Do you know why the page works in 5.0.3, even though it doesn't have this patch? You should explain it in the ChangeLog!
> Source/WebCore/platform/graphics/win/QTMovie.cpp:792 > + if (infoCD.componentSubType = 'm4a ') {
Did you mean ==? Is the trailing space in the string intentional? I'm surprised the compiler didn't complain about the assignment. (I'm going to wait to r+ this until I hear back from you.)
> Source/WebCore/platform/graphics/win/QTMovie.cpp:793 > + static bool eatm4aSpecialCase = false;
Maybe it should be eatM4ASpecialCase?
> Source/WebCore/platform/graphics/win/QTMovie.cpp:803 > + CFStringRef cfMimeType = CFStringCreateWithCString(0, "audio/m4a", kCFStringEncodingUTF8); > + if (!cfMimeType) > + continue; > + > + gSupportedTypes->append(cfMimeType); > + > + cfMimeType = CFStringCreateWithCString(0, "audio/x-m4a", kCFStringEncodingUTF8);
Why not use CFSTR() for these two cases?
Jer Noble
Comment 4
2011-01-17 09:07:21 PST
(In reply to
comment #3
)
> (From update of
attachment 79056
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=79056&action=review
> > Do you know why the page works in 5.0.3, even though it doesn't have this patch? You should explain it in the ChangeLog! > > > Source/WebCore/platform/graphics/win/QTMovie.cpp:792 > > + if (infoCD.componentSubType = 'm4a ') { > > Did you mean ==?
Wow! Horrible mistake on my part. That should be ==.
> Is the trailing space in the string intentional?
On the other hand, the extra space is intentional.
> I'm surprised the compiler didn't complain about the assignment. (I'm going to wait to r+ this until I hear back from you.)
I'll reupload after making these changes.
> > Source/WebCore/platform/graphics/win/QTMovie.cpp:793 > > + static bool eatm4aSpecialCase = false; > > Maybe it should be eatM4ASpecialCase?
Sure.
> > Source/WebCore/platform/graphics/win/QTMovie.cpp:803 > > + CFStringRef cfMimeType = CFStringCreateWithCString(0, "audio/m4a", kCFStringEncodingUTF8); > > + if (!cfMimeType) > > + continue; > > + > > + gSupportedTypes->append(cfMimeType); > > + > > + cfMimeType = CFStringCreateWithCString(0, "audio/x-m4a", kCFStringEncodingUTF8); > > Why not use CFSTR() for these two cases?
No good reason; I'll use CFSTR instead.
Jer Noble
Comment 5
2011-01-17 10:36:56 PST
(In reply to
comment #3
)
> (From update of
attachment 79056
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=79056&action=review
> > Do you know why the page works in 5.0.3, even though it doesn't have this patch? You should explain it in the ChangeLog!
If you run 5.0.3 against QuickTime 7.6.9, the problem still reproduces. I suspect this is a change in the list of MIME types which we get back from QuickTime.
Jer Noble
Comment 6
2011-01-17 10:40:09 PST
(continued...) Actually, it's probably a change in CoreAudio (which ships with the QuickTime installer, but lives inside AAS). I'll try to track down old versions to double check.
Jer Noble
Comment 7
2011-01-17 11:20:04 PST
Nevermind, looks like this was a regression caused by:
https://bugs.webkit.org/show_bug.cgi?id=49497
http://trac.webkit.org/changeset/72119
We started asking the system for extension->MIME type mappings; in the Windows case, this meant asking the Registry. QuickTime adds these mappings during install time, but uses a different mechanism than we do when querying components for their MIME types, so their mapping list is more extensive than the one we come up with during QTMovie::initializeSupportedTypes().
Jer Noble
Comment 8
2011-01-17 11:29:48 PST
Created
attachment 79193
[details]
Patch
Jer Noble
Comment 9
2011-01-17 11:35:25 PST
(In reply to
comment #8
)
> Created an attachment (id=79193) [details] > Patch
Whoops, this patch missed the ChangeLog, which adds the following paragraph: Caused by
r72119
, which adds system-specific extension->MIME entries to the cache before global entries, and the system-specific entries include QuickTime's registry entries which contain the audio/m4a MIME type, while its components do not.
Adam Roben (:aroben)
Comment 10
2011-01-17 11:52:03 PST
(In reply to
comment #7
)
> Nevermind, looks like this was a regression caused by: >
https://bugs.webkit.org/show_bug.cgi?id=49497
>
http://trac.webkit.org/changeset/72119
> > We started asking the system for extension->MIME type mappings; in the Windows case, this meant asking the Registry. QuickTime adds these mappings during install time, but uses a different mechanism than we do when querying components for their MIME types, so their mapping list is more extensive than the one we come up with during QTMovie::initializeSupportedTypes().
Thanks for tracking it down! That makes me feel a lot better. Are there other MIME types that will hit the same problem? Should we teach QTMovieWin how to read the registry the same way QuickTime does?
Jer Noble
Comment 11
2011-01-17 12:39:35 PST
(In reply to
comment #10
)
> (In reply to
comment #7
) > > Nevermind, looks like this was a regression caused by: > >
https://bugs.webkit.org/show_bug.cgi?id=49497
> >
http://trac.webkit.org/changeset/72119
> > > > We started asking the system for extension->MIME type mappings; in the Windows case, this meant asking the Registry. QuickTime adds these mappings during install time, but uses a different mechanism than we do when querying components for their MIME types, so their mapping list is more extensive than the one we come up with during QTMovie::initializeSupportedTypes(). > > Thanks for tracking it down! That makes me feel a lot better. > > Are there other MIME types that will hit the same problem? Should we teach QTMovieWin how to read the registry the same way QuickTime does?
I talked about this with Eric; the code which extracts MIME types from the QuickTime components during installation is incredibly hairy. He would really prefer not pulling that code into WebKit. And while QuickTime writes to the registry during installation, there's no explicit guarantee that the version of QuickTime you're running against is the same as the version which wrote to the registry. We could also remove the section of the windows MediaPlayerPrivate which queries the Registry for extension->MIME mappings; since that seems to be the original cause of the regression.
Eric Carlson
Comment 12
2011-01-18 11:25:19 PST
(In reply to
comment #11
)
> (In reply to
comment #10
) > > > > Are there other MIME types that will hit the same problem? Should we teach QTMovieWin how to read the registry the same way QuickTime does? > > I talked about this with Eric; the code which extracts MIME types from the QuickTime components during installation is incredibly hairy. He would really prefer not pulling that code into WebKit. And while QuickTime writes to the registry during installation, there's no explicit guarantee that the version of QuickTime you're running against is the same as the version which wrote to the registry. >
Actually it turns out QuickTime has a helper component we can use that loads the "extended" MIME metadata from all installed components and unpacks it into a much more reasonable format. This is what the QuickTime installer uses to configure the registry, so we should use it as well so we end up with the same list of types.
Eric Carlson
Comment 13
2011-01-18 11:26:09 PST
Comment on
attachment 79193
[details]
Patch Clearing r? while we work on another approach
Jer Noble
Comment 14
2011-01-21 11:21:48 PST
Created
attachment 79765
[details]
Patch
Darin Adler
Comment 15
2011-01-21 11:30:21 PST
Comment on
attachment 79765
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=79765&action=review
EWS bot was unable to apply this patch.
> Source/WebCore/ChangeLog:41 > +2011-01-20 Jer Noble <
jer.noble@apple.com
> > + > + Reviewed by NOBODY (OOPS!). > + > + REGRESSION (
r72119
): Audio never plays on Star Wars intro animation > +
https://bugs.webkit.org/show_bug.cgi?id=52467
> + > + QuickTime's eat/m4a movie importer compontent doesn't list audio/m4a as a mime > + type which it supports, though it handles .m4a files just fine. Change the way > + we build the list of supported MIME Types through a new WebKitSystemInterface > + function. > + > + Caused by
r72119
, which adds system-specific extension->MIME entries to the cache > + before global entries, and the system-specific entries include QuickTime's registry > + entries which contain the audio/m4a MIME type, while its components do not. > + > + Test: media/audio-mpeg4-supported.html > + > + * WebCore.vcproj/QTMovieWinCommon.vsprops: > + * platform/graphics/win/QTMovie.cpp: > + (getMIMETypeCallBack): > + (initializeSupportedTypes): > + > +2011-01-14 Jer Noble <
jer.noble@apple.com
> > + > + Reviewed by NOBODY (OOPS!). > + > + REGRESSION (
r72119
): Audio never plays on Star Wars intro animation (52467) > +
https://bugs.webkit.org/show_bug.cgi?id=52467
> + > + QuickTime's eat/m4a movie importer compontent doesn't list audio/m4a as a mime > + type which it supports, though it handles .m4a files just fine; so special case > + the mime types for that component. > + > + Caused by
r72119
, which adds system-specific extension->MIME entries to the cache > + before global entries, and the system-specific entries include QuickTime's registry > + entries which contain the audio/m4a MIME type, while its components do not. > + > + * platform/graphics/win/QTMovie.cpp: > + (initializeSupportedTypes): > +
Double change log.
> Source/WebCore/platform/graphics/win/QTMovie.cpp:753 > + CFStringRef cfType = CFStringCreateWithCString(kCFAllocatorDefault, type, kCFStringEncodingMacRoman);
MacRoman? Really? If it’s always going to be ASCII then can we say that? Or use Latin1? Or UTF-8?
> WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:172 > +typedef void(*wkQuickTimeMIMETypeCallBack)(const char* mimeType);
I think there should be a space after void. I’m surprised that we are not capitalizing the WK in the typedef.
Jer Noble
Comment 16
2011-01-21 12:16:12 PST
(In reply to
comment #15
)
> (From update of
attachment 79765
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=79765&action=review
> > EWS bot was unable to apply this patch.
"Original content of WebKitLibraries/win/lib/WebKitSystemInterface.lib mismatches at /mnt/git/webkit-chromium-ews/Tools/Scripts/svn-apply " Weird. Perhaps I was supposed to trim out the .lib file from the patch before uploading?
> > Double change log.
Whoops! Removed.
> > Source/WebCore/platform/graphics/win/QTMovie.cpp:753 > > + CFStringRef cfType = CFStringCreateWithCString(kCFAllocatorDefault, type, kCFStringEncodingMacRoman); > > MacRoman? Really? If it’s always going to be ASCII then can we say that? Or use Latin1? Or UTF-8?
Believe it or not, but the strings we pull from the PluginHelper are encoded in MacRoman. No really. :)
> > WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:172 > > +typedef void(*wkQuickTimeMIMETypeCallBack)(const char* mimeType); > > I think there should be a space after void. I’m surprised that we are not capitalizing the WK in the typedef.
Enspaced. There was already one typedef'd callback function in the WebKitSystemInterface.h that was prefixed with a lower-case "wk", so I just followed the convention. The convention could be wrong though.
Jer Noble
Comment 17
2011-01-21 12:31:52 PST
(In reply to
comment #16
)
> (In reply to
comment #15
) > > > Source/WebCore/platform/graphics/win/QTMovie.cpp:753 > > > + CFStringRef cfType = CFStringCreateWithCString(kCFAllocatorDefault, type, kCFStringEncodingMacRoman); > > > > MacRoman? Really? If it’s always going to be ASCII then can we say that? Or use Latin1? Or UTF-8? > > Believe it or not, but the strings we pull from the PluginHelper are encoded in MacRoman. No really. :)
One more thing: since MacRoman is a superset of ASCII, CFString will scan the string and if it finds all ASCII characters it will just store the string as ASCII. So in the end it amounts to the same thing, except for any weird edge cases where a MIME Type has a MacRoman character included.
Eric Carlson
Comment 18
2011-01-21 13:04:01 PST
Comment on
attachment 79765
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=79765&action=review
Clearing r+ flag for now, this needs some more work.
> Source/WebCore/platform/graphics/win/QTMovie.cpp:-813 > - // Only add "audio/..." and "video/..." types. > - if (strncmp(typeBuffer, "audio/", 6) && strncmp(typeBuffer, "video/", 6)) > - continue;
This filtering is important, QuickTime can open *lots* of MIME types we don't want the media element to support.
> Source/WebCore/platform/graphics/win/QTMovie.cpp:-831 > - // Only add each type once. > - bool alreadyAdded = false; > - for (int addedIndex = 0; addedIndex < gSupportedTypes->size(); addedIndex++) { > - CFStringRef type = gSupportedTypes->at(addedIndex); > - if (kCFCompareEqualTo == CFStringCompare(cfMimeType, type, kCFCompareCaseInsensitive)) { > - alreadyAdded = true; > - break; > - } > - } > - if (!alreadyAdded) > - gSupportedTypes->append(cfMimeType); > - else > - CFRelease(cfMimeType);
The new code should only add each type to the array once.
Jer Noble
Comment 19
2011-01-21 17:05:45 PST
Created
attachment 79811
[details]
Patch
Eric Carlson
Comment 20
2011-01-24 10:48:03 PST
Comment on
attachment 79811
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=79811&action=review
> Source/WebCore/ChangeLog:10 > + we build the list of supported MIME Types through a new WebKitSystemInterface
Minor nit - "Types" doesn't need to be capitalized.
Jer Noble
Comment 21
2011-01-25 11:54:24 PST
Committed
r76621
: <
http://trac.webkit.org/changeset/76621
>
WebKit Review Bot
Comment 22
2011-01-25 12:22:38 PST
http://trac.webkit.org/changeset/76621
might have broken Qt Linux Release The following tests are not passing: media/video-source-moved.html
Adam Roben (:aroben)
Comment 23
2011-03-21 11:38:40 PDT
I can still reproduce in the latest nightly.
Jer Noble
Comment 24
2011-05-05 10:49:55 PDT
(In reply to
comment #23
)
> I can still reproduce in the latest nightly.
I've filed
bug #60229
to track the new issue.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug