If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[Video] MPEG-4 + AMR_NB in 3gp container is not playing

RESOLVED WONTFIX

Status

Firefox OS
Gaia::Video
RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: mbarone, Assigned: eflores)

Tracking

(Blocks: 1 bug, {feature})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 765810 [details]
media file

STR

Open Video app and try to reproduce a media file (MPEG-4+AMR_NB) in 3gp container (attached the media)

EXPECTED RESULT
The media file must be played

ACTUAL RESULT
The media file is not played

This format blocks the Brasil certification, so I nominate it.
Adding needsinfo on :roc to see if this is the similar support we have in desktop as of today in which case this would not be a blocker.
We don't support any kind of 3GP on desktop.

I don't think we want to support AMR in 3GP, but that's up to Chris D.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> I don't think we want to support AMR in 3GP, but that's up to Chris D.
Flags: needinfo?(chris.double)

Comment 4

4 years ago
(In reply to Andrew Overholt [:overholt] from comment #3)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> > I don't think we want to support AMR in 3GP, but that's up to Chris D.

Given that we are not currently supporting 3GP on desktop at all, I think the questions is "Do we support AMR in 3GP on mobile" and the answer to that is yes, otherwise MMS doesn't work.
Flags: needinfo?(chris.double)

Comment 5

4 years ago
Sandip - can you confirm comment 4? If this is critical for v1.1 MMS, I'd be surprised that it was coming up this late in the game.
Flags: needinfo?(skamat)
Keywords: feature

Comment 6

4 years ago
If I recall correctly the media apps do parsing of the media file to obtain content information and if it is not able to parse the content it ignores it. In particular it doesn't handle some possible 'ftypes' See for example bug 882683 and bug 859711.
Triage - leo+ per comments from partner & certification perspective.
This format combination is used for MMS - which we support
blocking-b2g: leo? → leo+

Comment 8

4 years ago
(In reply to Alex Keybl [:akeybl] from comment #5)
> Sandip - can you confirm comment 4? If this is critical for v1.1 MMS, I'd be
> surprised that it was coming up this late in the game.

yes, Confirming comment #4. The issue is not of that format being a new requirement. It is actually looks to be what Chris has articulated in comment #6. The parsing of this type of content needs to be supported in the media apps.
Flags: needinfo?(skamat)
If this is required for MMS then is it something that can be assigned/worked on/fixed during this week's workweek?
Flags: needinfo?(dkuo)
Flags: needinfo?(dietrich)

Comment 10

4 years ago
Created attachment 770092 [details]
screenshot of video app

Actually video app use video.canPlayType() to check a file is playable or not, and for attachment 765810 [details], gecko returns "true" so that Video app scans and displays attachment 765810 [details], and I was able to play it so apparently this is not a gaia issue. But I found that video will always stuck on the first frame, I guess this should be a decoding issue so we need some help from gecko.
Flags: needinfo?(dkuo)
Edwin, can you take this?
Assignee: nobody → edwin
Renom. We just minused bug 882669 and closed it as not valid for the other AMR type. Is this not valid as well?
blocking-b2g: leo+ → leo?
Flags: needinfo?(clee)
(In reply to Jason Smith [:jsmith] from comment #17)
> Renom. We just minused bug 882669 and closed it as not valid for the other
> AMR type. Is this not valid as well?

Meant to say bug 882689 there.
Triage - leo- per comment from bug 882689 - the format will be supported within the MMS/Telephony apps only.

Chris/Noemi please renom if this is misunderstood.

(Noemi to check carrier cert requirement on mms)
blocking-b2g: leo? → ---
Flags: needinfo?(noef)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Edwin, can you take this?
Looking at this now.
Status: NEW → ASSIGNED
Well this is messed up.

Looks like we're queuing too many buffers in the video queue, so that OMXCodec ends up waiting for us to release a buffer, which happens on _the next frame draw_. Since we alternate between video and audio decode on the same thread, we end up being able to decode only one audio sample per frame drawn. Then, because the duration of each audio sample is shorter than the video frames' duration, the audio eventually falls behind.

So we try to A/V sync by not drawing any new video frames and waiting for audio samples. Which brings us back to audio decode blocking on video decode. Which blocks on video drawing. One big cycle of sad face.

I guess we just have to *yet again* lower AmpleVideoFrames; perhaps depending on which OMX component is being used.
Created attachment 774463 [details] [diff] [review]
Lower AmpleVideoFrames for qcom OMX mpeg4 decoder

Patch enables playback at least. Video still messes up after seek; trying to track that down now.
Attachment #774463 - Flags: review?(chris.double)

Comment 18

4 years ago
Sotaro had a patch some time ago that lowered ample video frames and it had to be reversed IIRC. I've asked him for feedback on this fix.
Flags: needinfo?(sotaro.ikeda.g)

Updated

4 years ago
Attachment #774463 - Flags: feedback?(sotaro.ikeda.g)
If that's the case, there's not much we can do about this bug short of copying the video frames into the container which really defeats the purpose of having the zero copy path for mp4v.
The ample video frames is reversed in Bug 863441.
Flags: needinfo?(sotaro.ikeda.g)
Edwin, I think you use unagi for debugging right? Does your environment apply attachment 744257 [details] [diff] [review] in Bug 864230? The patch applies 2 more buffers to gecko. It is enabled only on ICS_STRAWBERRY(hamachi, buri, leo), because android::OMXCodec is out side of our code. It is the area of codeaurora. And codeaurora apply a support of it on only ICS_STRAWBERRY.

attachment 765810 [details] could be played normally on leo device. And when I applied attachment 744257 [details] [diff] [review] on unagi, the unagi can also play the video.
If my understanding is correct, attachment 774463 [details] [diff] [review] is not necessary.
mbarone, do you know if the device apply attachment 744257 [details] [diff] [review]?
Flags: needinfo?(mbarone976)

Comment 24

4 years ago
Comment on attachment 774463 [details] [diff] [review]
Lower AmpleVideoFrames for qcom OMX mpeg4 decoder

Removing review while discussion between Edwin and Sotaro continues. Re-request if it's still decided that it's needed.
Attachment #774463 - Flags: review?(chris.double)
(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> attachment 765810 [details] could be played normally on leo device. And when
> I applied attachment 744257 [details] [diff] [review] on unagi, the unagi
> can also play the video.

I asked Chris on IRC about that patch and he said it was backed out due to breakage on some devices. Needinfo'ing.
Flags: needinfo?(chris.double)

Comment 26

4 years ago
(In reply to Edwin Flores [:eflores] [:edwin] from comment #25)
> (In reply to Sotaro Ikeda [:sotaro] from comment #21)
> > attachment 765810 [details] could be played normally on leo device. And when
> > I applied attachment 744257 [details] [diff] [review] on unagi, the unagi
> > can also play the video.
> 
> I asked Chris on IRC about that patch and he said it was backed out due to
> breakage on some devices. Needinfo'ing.

Which Chris did you ask on IRC? Needinfo Sotaro as he was involved in the backout, not me.
Flags: needinfo?(chris.double) → needinfo?(sotaro.ikeda.g)
This is the backout I was thinking of:
https://bugzilla.mozilla.org/show_bug.cgi?id=832653#c12

I'm guessing it was because of minUndequeuedBuffers differing between decoder implementations. attachment 744257 [details] [diff] [review] should be fine, then.

What device is this happening on? We should make sure that patch has been applied on it. Or rather, not applied, currently.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #27)
> This is the backout I was thinking of:
> https://bugzilla.mozilla.org/show_bug.cgi?id=832653#c12

attachment 744257 [details] [diff] [review] is not backed out. Bug 864230 comment #80 explains about the patch.

attachment 734527 [details] [diff] [review] in the above link is to increase the allocation buffer by hw codec. This way of change could fail on some devides. It is actually failed in unagi devices and v1.01 devices. Bug 869443 was similar bug that tried to increase the hw codec allocated buffers.
Flags: needinfo?(sotaro.ikeda.g)
attachment 744257 [details] [diff] [review] is maintained as patches made by codeaurora. It is applyed to source code during build.

https://github.com/mozilla-b2g/gonk-patches/blob/master/all-leo/frameworks/base/0001-Don-t-let-OMXCodec-undequeue-any-buffers.patch
Comment on attachment 774463 [details] [diff] [review]
Lower AmpleVideoFrames for qcom OMX mpeg4 decoder

Clear feed back. I already commented to the patch in comment #22.
Attachment #774463 - Flags: feedback?(sotaro.ikeda.g)
Moving to ffos-product to see if anyone on the group can respond on the needinfo request here.
Flags: needinfo?(clee) → needinfo?(ffos-product)

Comment 32

4 years ago
(In reply to Jason Smith [:jsmith] from comment #31)
> Moving to ffos-product to see if anyone on the group can respond on the
> needinfo request here.

Hi Jason, what specifically is the info needed from ffos-product? sorry if I missed.
(In reply to Sandip Kamat from comment #32)
> (In reply to Jason Smith [:jsmith] from comment #31)
> > Moving to ffos-product to see if anyone on the group can respond on the
> > needinfo request here.
> 
> Hi Jason, what specifically is the info needed from ffos-product? sorry if I
> missed.

I think I was trying to find out if we need to support the MPEG-4 + AMR_NB use case in a 3GP container for 1.1 or not. If we need to support this use case, then we should renom this to block. Otherwise, we can punt on this to a future release.

Based on my comments above though, this appears to be a feature though that we have not implemented support for yet.
(In reply to Wayne Chang [:wchang] from comment #14)
> Triage - leo- per comment from bug 882689 - the format will be supported
> within the MMS/Telephony apps only.
> 
> Chris/Noemi please renom if this is misunderstood.
> 
> (Noemi to check carrier cert requirement on mms)

Hi,

It wouldn't be a blocker for v1.1 but it would be needed in future versions since it is part of BUs certification process.
Flags: needinfo?(noef)

Comment 35

4 years ago
(In reply to Jason Smith [:jsmith] from comment #33)
> (In reply to Sandip Kamat from comment #32)
> > (In reply to Jason Smith [:jsmith] from comment #31)
> > > Moving to ffos-product to see if anyone on the group can respond on the
> > > needinfo request here.
> > 
> > Hi Jason, what specifically is the info needed from ffos-product? sorry if I
> > missed.
> 
> I think I was trying to find out if we need to support the MPEG-4 + AMR_NB
> use case in a 3GP container for 1.1 or not. If we need to support this use
> case, then we should renom this to block. Otherwise, we can punt on this to
> a future release.
> 
> Based on my comments above though, this appears to be a feature though that
> we have not implemented support for yet.

We are okay for 1.1 as long as MMS case is supported. Other support can go to 1.2.

Updated

4 years ago
Flags: needinfo?(ffos-product)
Flags: needinfo?(dietrich)
So does this require any action on the Mozilla side? Seems like it's up to upstream here.
Flags: needinfo?(sotaro.ikeda.g)
Mozilla side do not need action until we get new information. This problem should be fixed by Bug 864230. But we did not got information whether Bug 864230 fix the problem.
Flags: needinfo?(sotaro.ikeda.g)

Comment 38

4 years ago
We continue with problems with playing the video.
Before, the video was blocked when it was opened by the user. After landing the bug 864230, the video is blocked when it is open, but sometimes progress very slow .
Status: ASSIGNED → NEW
Flags: needinfo?(mbarone976) → needinfo?

Updated

4 years ago
Flags: needinfo?

Comment 39

4 years ago
For the comment 38. The bug was tested with a unagi device with master branch and the commits Gecko-9fc67ed Gaia-845abbb
master branch is not good for testing video by Bug 901220 and Bug 901224. Can you confirm by v1.1?
Flags: needinfo?(rafael.marquez)

Comment 41

4 years ago
I have tested the bug with 1.1 branch and is not solved. The video is blocked and not progress. I used the commits: Gecko-b705197 Gaia-c60e350
Flags: needinfo?(rafael.marquez)
(In reply to rafael.marquez from comment #41)
> I have tested the bug with 1.1 branch and is not solved. The video is
> blocked and not progress. I used the commits: Gecko-b705197 Gaia-c60e350

Do you know whether the ROM include bug 864230?

In my case, when I locally applied the patch to unagi, the problem was fix. The patch is out side of gecko/gaia area. The gecko commit tag do not provide information about the patch. And qcom do not provide support for the patch in ics_chocolate platform.
Flags: needinfo?(rafael.marquez)

Comment 43

4 years ago
I do not understand your question. These commits are from the 1.1 branch of day 08/13/2013. I think the bug is included in that build
Flags: needinfo?(rafael.marquez)
(In reply to rafael.marquez from comment #43)
> I do not understand your question. These commits are from the 1.1 branch of
> day 08/13/2013. I think the bug is included in that build

Which device are you using to test? If your device is unagi, inari, your ROM should not include the fix of bug 864230 even when your ROM is recent v1.1. Because the code fix is in the area of codeaurora. unagi and inari uses codeaurora's ics_chocolate as a base source code. ics_chocolate does not provide the fix of bug 864230, because qcom does not provide support for it.

hamachi, leo, helix uses codeaurora's ics_strawberry as the base source. ics_strawberry is newer code than ics_chocolate. Only ics_strawberry has fix of bug 864230. Therefore only hamachi, leo, helix device has a fix of bug 864230.
Flags: needinfo?(rafael.marquez)

Comment 45

4 years ago
I tested the Bug on a device D300_V07j_00_AUG_05_TLF_ES LG to build and is working correctly. 
If you think that it is correct that work properly in the LG, but not in the Unagi you can close the bug.
Flags: needinfo?(rafael.marquez)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID

Comment 46

4 years ago
Edwin can you explain me why the bug is invalid
(In reply to rafael.marquez from comment #46)
> Edwin can you explain me why the bug is invalid

The bug is in an upstream branch (chocolate) which is unsupported by them. The bug is fixed in the strawberry branch, which *is* supported by upstream and used by all current release devices AFAIK.

It's not worth fixing this on the Unagi, which uses the chocolate branch, which nobody really cares about.

I guess that makes this more of a WONTFIX.

Feel free to reopen if I'm wrong about chocolate not being used on devices we care about.
Resolution: INVALID → WONTFIX
You need to log in before you can comment on or make changes to this bug.