[FFOS] Support avi/divx, ts/m2ts, and mkv container formats

RESOLVED FIXED in 2.2 S3 (9jan)

Status

enhancement
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: vasanth, Assigned: bwu)

Tracking

unspecified
2.2 S3 (9jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+)

Details

Attachments

(4 attachments, 5 obsolete attachments)

Reporter

Description

5 years ago
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.

Comment 1

5 years ago
Test patch for discovering additional container formats

Comment 2

5 years ago
Gecko patch for discovering additional container formats

Comment 3

5 years ago
Uploaded WIP patches for changes to consider additional container types

the change is gaurded MOZ_OMX_EXTENDED_CONTAINERS flag for smooth support

Updated

5 years ago

Updated

5 years ago
blocking-b2g: --- → 2.2?

Comment 4

5 years ago
(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

Comment 5

5 years ago
Thanks Bhargav! 

Please add dhylands for reviewing gecko changes and djf for music/video gaia changes

Thanks
Hema
Severity: normal → enhancement
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?

Updated

5 years ago
Attachment #8502677 - Flags: review?(dflanagan)

Updated

5 years ago
Attachment #8502678 - Flags: review?(dhylands)
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.
Flags: needinfo?(bhargavg1)
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.
Attachment #8502678 - Flags: review?(dhylands) → review?(cajbir.bugzilla)
And yeah - cajbir will want tests for each of these formats. I added some for .3g2 in bug 986331

Comment 9

5 years ago
(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.
Flags: needinfo?(bhargavg1)

Comment 10

5 years ago
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.
Attachment #8502677 - Flags: review?(dflanagan)

Comment 12

5 years ago
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.
Attachment #8502678 - Flags: review?(cajbir.bugzilla)
The patch also has insufficient context. It should be exported with -U8.

Updated

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

Anyone?
Flags: needinfo?(bhargavg1)

Comment 15

5 years ago
(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
Flags: needinfo?(bhargavg1)
Passing to Bhargav as he is helping here.
Assignee: nobody → bhargavg1

Comment 17

5 years ago
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.
Attachment #8502678 - Attachment is obsolete: true
Attachment #8533266 - Flags: review?(dflanagan)
Added more context
Attachment #8502677 - Attachment is obsolete: true
Attachment #8533267 - Flags: review?(dflanagan)
Attachment #8533266 - Flags: review?(dflanagan) → review?(dhylands)
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.
Attachment #8533267 - Flags: review?(dflanagan) → review-
Also note that for Gaia changes, we'll want the patch to be in the form of a github pull request.
Assignee: bhargavg1 → nobody
No longer blocks: CAF-v2.2-metabug
Assignee

Comment 22

5 years ago
Hi Nicholas, 
Will you continue working on this bug?
If no, I can take this bug to move it forward.
Flags: needinfo?(ntroast)
(Nick was just helping with comment 13, but otherwise isn't actively moving this forward)
Flags: needinfo?(ntroast)
Assignee

Comment 24

5 years ago
(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.
Assignee: nobody → bwu
Assignee

Updated

5 years ago
Summary: Discovering additional video/audio container types → [FFOS] Support avi/divx, ts/m2ts, and mkv media formats
Assignee

Comment 25

5 years ago
Posted video avi test content
avi test content
Assignee

Comment 26

5 years ago
Posted video mkv test content
mkv test content
Assignee

Comment 27

5 years ago
Posted file .ts test content
.ts test content
Assignee

Comment 28

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

Updated

5 years ago
feature-b2g: 2.2? → 2.2+
Assignee

Comment 29

5 years ago
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
Attachment #8533266 - Attachment is obsolete: true
Attachment #8533267 - Attachment is obsolete: true
Attachment #8533266 - Flags: review?(dhylands)
Attachment #8537048 - Flags: review?(dhylands)
Attachment #8537048 - Flags: review?(cajbir.bugzilla)
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.
Attachment #8537048 - Flags: review?(dhylands) → feedback+
Assignee

Comment 31

5 years ago
(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!
Assignee

Comment 32

5 years ago
To be more clear, 
This bug is intended to support more container formats, not codec formats.
Summary: [FFOS] Support avi/divx, ts/m2ts, and mkv media formats → [FFOS] Support avi/divx, ts/m2ts, and mkv container formats
Assignee

Comment 33

5 years ago
Hi Vasanth, 
AFAIK, CAF supports avi/divx demuxing which Android doesn't, right?
Flags: needinfo?(vasanth)
Assignee

Comment 34

5 years ago
(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 35

5 years ago
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.
Attachment #8537048 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Comment 36

5 years ago
1. Carry r+ from cajbir. 
2. Changes according to comment 35.
Attachment #8537048 - Attachment is obsolete: true
Attachment #8540492 - Flags: review+
Assignee

Comment 37

5 years ago
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.

Updated

5 years ago
Target Milestone: --- → 2.2 S3 (9jan)
https://hg.mozilla.org/mozilla-central/rev/2f11f87d0dea
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Assignee

Updated

3 years ago
Flags: needinfo?(vasanth)
You need to log in before you can comment on or make changes to this bug.