815226 - Make the MediaPluginReader not depend on the concrete type of the AbstractMediaDecoder passed to it

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

My fix to bug 813637 was sort of a dirty hack which will actually fail when we have decoder objects which are not MediaPluginDecoders.  We should just put GetContentType in a C++ interface and try to QI for it.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 685251 [details] [diff] [review]
Patch (v1)

Review of attachment 685251 [details] [diff] [review]:
-----------------------------------------------------------------

Could you instead just pass MediaPluginDecoder::mType to the MediaPluginReader constructor, and use that to init MediaPluginReader::mType, rather than retrieving it from MediaPluginDecoder?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Assuming what I said in comment 2 works, please also add a comment to warn against regressing this.

I still don't get why the cast from bug 813637 didn't work.

You could also try changing the type passed into MediaPluginReader::MediaPluginReader from AbstractMediaDecoder to MediaPluginDecoder, then the cast would happen in code that understands the layout of MediaPluginDecoder?

Comment 4

[(no longer active)]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Assuming what I said in comment 2 works, please also add a comment to warn
> against regressing this.

OK, let me see what I can do.

> I still don't get why the cast from bug 813637 didn't work.

AbstractMediaDecoder is the second base of MediaDecoder, so the cast should modify the address stored in the pointer to be one word less than the value passed to the function, which static_cast does correctly, but reinterpret_cast doesn't.

The reason that static_cast is not the right fix is that it's assuming that it knows what the concrete type of the object passed to MediaPluginReader::MediaPluginReader is, which is the wrong assumption when my decodeAudioData stuff lands.

> You could also try changing the type passed into
> MediaPluginReader::MediaPluginReader from AbstractMediaDecoder to
> MediaPluginDecoder, then the cast would happen in code that understands the
> layout of MediaPluginDecoder?

Well I could do that, but that would defeat the purpose of AbstractMediaDecoder entirely, and prevent me from using MediaPluginReader for web audio.  But I think just making the constructor take the content type as a second argument would work fine.  I'll attach a patch soon.

Comment 5

[(no longer active)]

Attachment: Patch (v2)

OK, this is definitely much better!

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 685302 [details] [diff] [review]
Patch (v2)

Review of attachment 685302 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet.

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d66f03cc49a9

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d66f03cc49a9