We see MSM platforms supports some video containers (Ex: avi, ts, mkv) but Firefox OS video app does not recognize these types. We can use bug 986331 fix as a template to add these video containers and corresponding mime types to enable support for these formats in Firefox OS. I will try to upload gaia/gecko patch to achieve this.

Test patch for discovering additional container formats

Gecko patch for discovering additional container formats

Uploaded WIP patches for changes to consider additional container types the change is gaurded MOZ_OMX_EXTENDED_CONTAINERS flag for smooth support

(In reply to vasanth from comment #0) > We see MSM platforms supports some video containers (Ex: avi, ts, mkv) but > Firefox OS video app does not recognize these types. Additional containers that would be discovered are, avi/divx, ts/m2ts, mkv

Thanks Bhargav! Please add dhylands for reviewing gecko changes and djf for music/video gaia changes Thanks Hema

Bhargav, Do you have copyright-free sample media files in each of these formats? If we're going to add support to the Music and Video apps, I'd like to check in test files for each of the formats as well.

Comment on attachment 8502678 [details] [diff] [review] Gecko-Enable-discovering-ts-avi-and-mkv-media-formats.patch Review of attachment 8502678 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, but I think that cajbir needs to review this.

And yeah - cajbir will want tests for each of these formats. I added some for .3g2 in bug 986331

(In reply to David Flanagan [:djf] from comment #6) > Bhargav, > > Do you have copyright-free sample media files in each of these formats? If > we're going to add support to the Music and Video apps, I'd like to check in > test files for each of the formats as well. Dont have any such free sample videos.

Is the intent that these formats will only work for privileged apps?

Comment on attachment 8502677 [details] [diff] [review] Gaia-Enable-discovering-ts-avi-and-mkv-media-formats.patch Cancelling the review request for the gaia patch. Once we know that gecko can play files of these types and once we have some test files, we can consider the gaia patches. Jim Porter can review the music patch and Russ Nicoletti can review the video app patch. But I would like someone to justify the patch by explaining what these container formats are, where they are commonly used, and why our users will care about them. That, plus a links to files we can test against, please. Note that the Music and Video apps are likely to work with just the necessary gecko patches in place. The proposed gaia patches look like they are mainly for the pick, share and view activities. But basic scanning will work with the device storage patch. And if gecko can play these formats, then the gaia apps should be able to play them once they can find them when scanning.

Comment on attachment 8502678 [details] [diff] [review] Gecko-Enable-discovering-ts-avi-and-mkv-media-formats.patch Awaiting answer on comment 10 before reviewing. Please re-add review when answered.

The patch also has insufficient context. It should be exported with -U8.

(In reply to cajbir (:cajbir) from comment #10) > Is the intent that these formats will only work for privileged apps? Anyone?

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #14) > (In reply to cajbir (:cajbir) from comment #10) > > Is the intent that these formats will only work for privileged apps? > > Anyone? we already have codecs available for these formats accessible as the way others are. if there are any limitations from the Gecko I am not sure

Passing to Bhargav as he is helping here.

We'll upload another patch with more context, but the patch needs re-work due to bug 946065.

Reworked patch due to bug 946065. Added more context.

Added more context

Comment on attachment 8533267 [details] [diff] [review] Gaia-Enable-discovering-ts-avi-and-mkv-media-formats.patch Review of attachment 8533267 [details] [diff] [review]: ----------------------------------------------------------------- Please see comment 11. I want to insist on sample files for these formats as part of the patch. Music samples in apps/music/test/samples/ and video samples in apps/video/test/videos/ perhaps. Also, please ask Jim Porter [:squib] to review the music patch once samples are included. I can review the video patch. I recommend that you narrow the scope of this bug to just getting the gecko changes landed. Then, once those are in the tree, file two new bugs for the Music app and Video app changes.

Also note that for Gaia changes, we'll want the patch to be in the form of a github pull request.

Hi Nicholas, Will you continue working on this bug? If no, I can take this bug to move it forward.

(Nick was just helping with comment 13, but otherwise isn't actively moving this forward)

(In reply to Michael Vines [:m1] [:evilmachines] from comment #23) > (Nick was just helping with comment 13, but otherwise isn't actively moving > this forward) Thanks for the info.

avi test content

mkv test content

.ts test content

For divx content, some content is available on http://www.divx.com/en/devices/profiles/video.

These formats will be only work in webapps, not in broswer as Audio AMR[1]. [1] http://dxr.mozilla.org/mozilla-central/source/dom/media/DecoderTraits.cpp?from=DecoderTraits.cpp#546

Comment on attachment 8537048 [details] [diff] [review] Bug-1080484-Support-avi-divx-ts-m2ts-and-mkv-media-f.patch Review of attachment 8537048 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, although the test files should be attached to the patch, not just the bug.

(In reply to Dave Hylands [:dhylands]{PTO until Jan 5) from comment #30) > Comment on attachment 8537048 [details] [diff] [review] > Bug-1080484-Support-avi-divx-ts-m2ts-and-mkv-media-f.patch > > Review of attachment 8537048 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks fine to me, although the test files should be attached to the > patch, not just the bug. Thanks for your reminder!

To be more clear, This bug is intended to support more container formats, not codec formats.

Hi Vasanth, AFAIK, CAF supports avi/divx demuxing which Android doesn't, right?

(In reply to Dave Hylands [:dhylands]{PTO until Jan 5) from comment #30) > Comment on attachment 8537048 [details] [diff] [review] > Bug-1080484-Support-avi-divx-ts-m2ts-and-mkv-media-f.patch > > Review of attachment 8537048 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks fine to me, although the test files should be attached to the > patch, not just the bug. Just found currently our test cases are run via browser only. But these additional formats are only for webapps, so there should be no need to add them in test cases.

Comment on attachment 8537048 [details] [diff] [review] Bug-1080484-Support-avi-divx-ts-m2ts-and-mkv-media-f.patch Review of attachment 8537048 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/exthandler/nsExternalHelperAppService.cpp @@ +562,5 @@ > + { VIDEO_AVI, "divx", "Audio Video Interleave" }, > + { VIDEO_MPEG_TS, "ts", "MPEG Transport Stream" }, > + { VIDEO_MPEG_TS, "m2ts", "MPEG-2 Transport Stream" }, > + { VIDEO_MATROSKA, "mkv", "MATROSKA VIDEO" }, > + { AUDIO_MATROSKA, "mka", "MATROSKA AUDIO" }, Make these MOZ_WIDGET_GONK checked same as the AUDIO_AMR entry.

1. Carry r+ from cajbir. 2. Changes according to comment 35.

Thanks for the review! (In reply to cajbir (:cajbir) from comment #35) > Comment on attachment 8537048 [details] [diff] [review] > Bug-1080484-Support-avi-divx-ts-m2ts-and-mkv-media-f.patch > > Review of attachment 8537048 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: uriloader/exthandler/nsExternalHelperAppService.cpp > @@ +562,5 @@ > > + { VIDEO_AVI, "divx", "Audio Video Interleave" }, > > + { VIDEO_MPEG_TS, "ts", "MPEG Transport Stream" }, > > + { VIDEO_MPEG_TS, "m2ts", "MPEG-2 Transport Stream" }, > > + { VIDEO_MATROSKA, "mkv", "MATROSKA VIDEO" }, > > + { AUDIO_MATROSKA, "mka", "MATROSKA AUDIO" }, > > Make these MOZ_WIDGET_GONK checked same as the AUDIO_AMR entry.

Test cases look good! https://treeherder.mozilla.org/#/jobs?repo=try&revision=81bb5cd5551e