Closed Bug 1080484 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: Gaia::Video, enhancement)

Product:
Firefox OS Graveyard
Component:
Gaia::Video
Platform:
ARM
Gonk (Firefox OS)
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:2.2+)

Status:
RESOLVED FIXED
Milestone:
2.2 S3 (9jan)
Project Flags:
feature-b2g 2.2+

People

(Reporter: vasanth, Assigned: bwu)

References

Details

Attachments

(4 files, 5 obsolete files)

avi test content
1.58 MB, video/x-msvideo
Details
mkv test content
2.42 MB, video/x-matroska
Details
.ts test content
1.70 MB, text/vnd.trolltech.linguist
Details
Final-Bug-1080484-Support-avi-divx-ts-m2ts-and-mkv-media-f.patch
7.66 KB, patch
bwu
: review+
Details | Diff | Splinter Review 
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
Blocks: CAF-v2.2-metabug
blocking-b2g: --- → 2.2?
(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
Severity: normal → enhancement
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?
Attachment #8502677 - Flags: review?(dflanagan)
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
(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)
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 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.
Blocks: 1071484
(In reply to cajbir (:cajbir) from comment #10)
> Is the intent that these formats will only work for privileged apps?

Anyone?
Flags: needinfo?(bhargavg1)
(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
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
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)
(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
Summary: Discovering additional video/audio container types → [FFOS] Support avi/divx, ts/m2ts, and mkv media formats
Attached video avi test content — 
avi test content
Attached video mkv test content — 
mkv test content
Attached file .ts test content — 
.ts test content
For divx content, 
some content is available on http://www.divx.com/en/devices/profiles/video.
feature-b2g: 2.2? → 2.2+
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+
(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.
Summary: [FFOS] Support avi/divx, ts/m2ts, and mkv media formats → [FFOS] Support avi/divx, ts/m2ts, and mkv container formats
Hi Vasanth, 
AFAIK, CAF supports avi/divx demuxing which Android doesn't, right?
Flags: needinfo?(vasanth)
(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.
Attachment #8537048 - Flags: review?(cajbir.bugzilla) → review+
1. Carry r+ from cajbir. 
2. Changes according to comment 35.
Attachment #8537048 - Attachment is obsolete: true
Attachment #8540492 - Flags: review+
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.
Target Milestone: --- → 2.2 S3 (9jan)
Flags: needinfo?(vasanth)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: