1153731 - Integrate MP3TrackDemuxer into MP4Reader

Status: RESOLVED WONTFIX

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

This bug is for the work that'll integrate Eugene's new MP3TrackDemuxer (in bug 1093815) into the existing MP4Reader framework and enable it on Android L/5.x.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: wip

Pretty hacky so far, need to sync up with jya on his refactoring of the MP4Reader stuff.  So far this sets up everything necessary, but fails to parse the file I tested with (https://ia902508.us.archive.org/5/items/testmp3testfile/mpthreetest.mp3).

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: wip v1

Rebased on top of latest code and Eugene's updated patches in bug 1093815.  With media.mp3.enabled = true (to force the new MP3 demuxer to be used) and, media.fragmented-mp4.exposed = true and media.fragmented-mp4.ffmpeg.enabled = true (on Linux) to enable a PlatformDecoderModule that supports MP3, initial playback works.  Replaying/seeking doesn't work yet.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1153731-Add-MP3LegacyDemuxer-adapter.patch

I've split the patch into three parts: the MP3TrackDemuxer modification went into my patch in bug 1093815, I moved MP3LegacyDemuxer into its own file and the rest of the integration work is in the third patch.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1153731-Integrate-MP3TrackDemuxer-into-MP4Reader.patch

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1153731-Add-MP3LegacyDemuxer-adapter.patch

Rebased on new MP4Demuxer interface.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1153731-Integrate-MP3TrackDemuxer-into-MP4Reader.patch

Rebased on new MP4Demuxer interface.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8603051 [details] [diff] [review]
0005-Bug-1153731-Integrate-MP3TrackDemuxer-into-MP4Reader.patch

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

This is great...

Just one thing: this entire code path (MP4Reader) is about to disappear as we move to the new media architecture (bug 1148292)..

All we need is a MP3Demuxer that supports the MediaDataDemuxer API (bug 1154164). This is very similar to what this patch does

With just a MP3Demuxer that implement MediaDataDemuxer API ; The MP3 MediaDecoder would then just become something like:
nsRefPtr<MediaFormatDecoder> reader = new MediaFormatDecoder(new MP3Demuxer(GetResource())) 
return new MediaDecoderStateMachine(this, reader);

(like what bug 1156708 does to replace the MP4Decoder)

Happy to discuss with you on the best way to proceed.

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Just one thing: this entire code path (MP4Reader) is about to disappear as
> we move to the new media architecture (bug 1148292)..

Yeah, that's why aspects of the integration are pretty temporary/hacky in these patches, since I was either going to rebase on top of your refactoring or check in these changes with an eye to removing them shortly, depending on which order things landed.

Comment 9

[Matthew Gregan [:kinetik]]

Marking these patches obsolete as we're in the process of rebasing the changes onto the new interfaces introduced with MediaFormatReader.  Changes are in Eugen's git repo for now: https://github.com/eamsen/gecko-dev/commits/mp3-demuxer-rebase

Comment 10

[Eugen Sawin [:esawin]]

I'm going to spin off the integration into a new bug, so that we have the following dependencies

Implementation Integration
bug 1093815 -> bug 1153731
    |
    v
bug 1166779 -> bug 1168374

Comment 11

[Matthew Gregan [:kinetik]]

Unless I'm misunderstanding something, this bug is replaced by bug 1168374, since we're not planning to integrate into the old MP4Reader any longer.