REGRESSION (r72119): Audio never plays on Star Wars intro animation

• Component: Media
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://www.gesteves.com/experiments/starwars.html

The audio never plays. It worked just great in 5.0.3.

[Adam Roben (:aroben)]

<rdar://problem/8866928>

[Jer Noble]

Created attachment 79056 [details]
Patch

[Adam Roben (:aroben)]

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]

(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]

(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]

(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]

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]

Created attachment 79193 [details]
Patch

[Jer Noble]

(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)]

(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]

(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]

(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 on attachment 79193 [details]
Patch

Clearing r? while we work on another approach

[Jer Noble]

Created attachment 79765 [details]
Patch

[Darin Adler]

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
> [phone]  Jer Noble  <[e-mail]>
> +
> +        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):
> +
> [phone]  Jer Noble  <[e-mail]>
> +
> +        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]

(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]

(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 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]

Created attachment 79811 [details]
Patch

[Eric Carlson]

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]

Committed r76621: <http://trac.webkit.org/changeset/76621>

[WebKit Review Bot]

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)]

I can still reproduce in the latest nightly.

[Jer Noble]

(In reply to comment #23)
> I can still reproduce in the latest nightly.

I've filed bug #60229 to track the new issue.