997593 - Cannot playback a recorded mp4 video on Firefox OS

Status: VERIFIED FIXED

(Core :: Audio/Video: Recording, defect)

Description

[Jason Smith [:jsmith]]

Attachment: logcat

Build - 4/16/2014 Flame Device
Version - 2.0

STR

1. Go to http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/
2. Setup a stream for media recorder by file with:
** File: gizmo.mp4
** Media Type: video
** Mime Type: video/mp4
3. Select play on both media controls that appear
4. Select start recording
5. When the video playback finishes, try playing back the recorded video

Expected

The video should playback what was recorded.

Actual

The video fails to play.

Comment 1

[Jason Smith [:jsmith]]

Randy - Any ideas why we aren't able to playback a recorded video?

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

I test flame with this page and it works well. 
http://people.mozilla.org/~rlin/mr/MR.html
What's the different?

Comment 3

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #2)
> I test flame with this page and it works well. 
> http://people.mozilla.org/~rlin/mr/MR.html
> What's the different?

The test case you are using is making use of WebM, where as my test case is using mp4.

FWIW - Your test page works for me as well.

Comment 4

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

On b2g platform, the video is encoded in the mp4 container.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #4)
> On b2g platform, the video is encoded in the mp4 container.

Then I don't understand why your test page is working. That test page using WebM, not mp4.

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

ah, what do you mean my page use WebM? The input media source is from gUM, and the encoded result should be mp4.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #6)
> ah, what do you mean my page use WebM? The input media source is from gUM,
> and the encoded result should be mp4.

Ah I see why I got confused. Randell clarified this to me in IRC. gUM provides raw frames, where as media recorder sets the codec and mux.

Note - gUM video is only one way of recording video. My test case records video from an existing mp4 file. That's the difference between your test case & mine.

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

I mean MediaRecorder would record the video frame from gUM, produce the encoded data in mp4 container on b2g platform. That is the test page behavior. 
gUM->start->stop->playback result.
oh, I should change the titie, 
 Record Live Camera image into (WebM)->(video clips)

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

BTW, I will check what's different with your test and mime.

Comment 10

[Jason Smith [:jsmith]]

Can we check if this happens on 1.4?

Comment 11

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #10)
> Can we check if this happens on 1.4?

Also reproduces on 1.4.

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Sorry for late response. It looks like transcode mp4 clips has some problem.
Have you tested for transcoding on webM content?

Comment 13

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #12)
> Sorry for late response. It looks like transcode mp4 clips has some problem.
> Have you tested for transcoding on webM content?

On desktop - I've been able to record WebM video content for some of the files seen at http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/.

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

I mean playback an webm file and record it. :)

Comment 15

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #14)
> I mean playback an webm file and record it. :)

Yes, I've been able to do that with my test case on desktop.

Comment 16

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

TAKE AND SOLVE IT.

Comment 17

[Jason Smith [:jsmith]]

I'm going to followup with Ivan, CJ, and Marvin on whether we should pref off video recording due to this bug.

Comment 18

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Symptom: On b2g platform
GUM (video:true)->Media Recorder: OK
Video tag with WebM content->Media Recorder: OK
Video tag with MP4 content->Media Recorder: NG

Comment 19

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #18)
> Symptom: On b2g platform
> GUM (video:true)->Media Recorder: OK
> Video tag with WebM content->Media Recorder: OK
> Video tag with MP4 content->Media Recorder: NG

The video tag with mp4 content is supposed to be supported right now, right?

FWIW - I got the same results as well in my testing.

Comment 20

[Jason Smith [:jsmith]]

Discussed with Marvin offline - we're going to only support the two use cases already supported for now (WebM & gUM video true). mp4 is out of scope for 1.4 since it doesn't work.

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

I think it should block on 2.0...

Comment 22

[Jason Smith [:jsmith]]

Okay - let me send this through 2.0 triage.

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

After analysis the log, while fail to transcode some video, I found OMX encoder module (android part) 
output E/OMX-VENC-720p(  301): VIDIOC_REQBUFS CAPTURE_MPLANE Failed.
Guess it's the input video frame format/size problem.

Comment 24

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

kernel log on flame.
<6>[   52.890122] msm_vidc: 4: Opening video instance: ece71000, 1
<6>[   53.887559] msm_vidc: 4: Opening video instance: ec3fa000, 0
<7>[   53.898764] msm_vidc: 1: No more formats found
<7>[   53.898789] msm_vidc: 1: No more formats found
<7>[   53.971381] msm_vidc: 1: Failed to get reqbufs, -16

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The encoder receive a new color format from omx decoder: OMX_QCOM_COLOR_FormatYUV420PackedSemiPlanar32m = 0x7FA30C04, the OMXWrapper need to handle this.

Comment 26

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Would have to convert frame image from OMX_QCOM_COLOR_FormatYUV420PackedSemiPlanar32m->RGB->NV12->Encoder

Comment 27

[Jason Smith [:jsmith]]

Marvin & I discussed this over vidyo - this will be considered for a future release, so moving to backlog.

Comment 28

[John Lin [:jhlin][:jolin]]

Attachment: Color onversion for QCOM Venus NV12 format image.

QCOM H.264 HW decoder on Flame outputs some proprietary YUV format called Venus NV12 (documented in [1]). Add color conversion code for that. 

[1] https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/include/media/msm_media_info.h?h=b2g_kk_3.5

Comment 29

[Peter Chang[:pchang]]

Comment on attachment 8482481 [details] [diff] [review]
Color onversion for QCOM Venus NV12 format image.

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

LGTM if you address my comment.

::: content/media/encoder/TrackEncoder.cpp
@@ +197,5 @@
>      while (!iter.IsEnded()) {
>        VideoChunk chunk = *iter;
>        if (!chunk.IsNull()) {
>          gfx::IntSize imgsize = chunk.mFrame.GetImage()->GetSize();
>          gfxIntSize intrinsicSize = chunk.mFrame.GetIntrinsicSize();

What happen if other platforms pass their own yuv format here? I would prefer to keep this error handing but check more format.

::: content/media/omx/OMXCodecWrapper.cpp
@@ +355,5 @@
>        yuv.mCbSkip = 0;
>        ConvertPlanarYCbCrToNV12(&yuv, aDestination);
>        break;
> +    // From QCOM video decoder on Flame. See bug 997593.
> +    case GrallocImage::HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS:

If you add this format here, do we need to change the function name because it is not the generic 420_SP format?

Comment 30

[John Lin [:jhlin][:jolin]]

(In reply to peter chang[:pchang][:peter] from comment #29)
> Comment on attachment 8482481 [details] [diff] [review]
> Color onversion for QCOM Venus NV12 format image.
> 
> Review of attachment 8482481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM if you address my comment.
> 
> ::: content/media/encoder/TrackEncoder.cpp
> @@ +197,5 @@
> >      while (!iter.IsEnded()) {
> >        VideoChunk chunk = *iter;
> >        if (!chunk.IsNull()) {
> >          gfx::IntSize imgsize = chunk.mFrame.GetImage()->GetSize();
> >          gfxIntSize intrinsicSize = chunk.mFrame.GetIntrinsicSize();
> 
> What happen if other platforms pass their own yuv format here? I would
> prefer to keep this error handing but check more format.

 Actually the above code was added to reject transcoding (See bug 1000736) because HW decoder outputs GrallocImage (format == GRALLOC_PLANAR_YCBCR).
 And for GrallocImage, if some platform passes YUV format we don't currently support, OMXCodecWrapper will assert in ConvertGrallocImageToNV12() (in debug build) or return error (in release build).
 
 What's still missing is that OmxTrackEncoder should catch the error and report it to MediaEncoder. I'll update the patch to fix that.

> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +355,5 @@
> >        yuv.mCbSkip = 0;
> >        ConvertPlanarYCbCrToNV12(&yuv, aDestination);
> >        break;
> > +    // From QCOM video decoder on Flame. See bug 997593.
> > +    case GrallocImage::HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS:
> 
> If you add this format here, do we need to change the function name because
> it is not the generic 420_SP format?

 Do you mean ConvertGrallocImageToNV12? I think the name still makes sense since it converts the data in GrallocImage (regardless what YUV format it's in) to NV12.

Comment 31

[John Lin [:jhlin][:jolin]]

Attachment: Catch errors from OMXCodecWrapper and pass them to recorder.

Recorder won't be able to report error for unsupported format or size without this patch.

Comment 32

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8482481 [details] [diff] [review]
Color onversion for QCOM Venus NV12 format image.

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

::: content/media/encoder/TrackEncoder.cpp
@@ +197,5 @@
>      while (!iter.IsEnded()) {
>        VideoChunk chunk = *iter;
>        if (!chunk.IsNull()) {
>          gfx::IntSize imgsize = chunk.mFrame.GetImage()->GetSize();
>          gfxIntSize intrinsicSize = chunk.mFrame.GetIntrinsicSize();

We still need to block the unsupport image format and stop recording.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8482481 [details] [diff] [review]
Color onversion for QCOM Venus NV12 format image.

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

The rest is OK.

::: content/media/encoder/TrackEncoder.cpp
@@ +197,5 @@
>      while (!iter.IsEnded()) {
>        VideoChunk chunk = *iter;
>        if (!chunk.IsNull()) {
>          gfx::IntSize imgsize = chunk.mFrame.GetImage()->GetSize();
>          gfxIntSize intrinsicSize = chunk.mFrame.GetIntrinsicSize();

Let's keep this check for now.

This Venus format is a form of PLANAR_YCBCR (right?) so keeping this check shouldn't cause problems.

Comment 34

[John Lin [:jhlin][:jolin]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Comment on attachment 8482481 [details] [diff] [review]
> Color onversion for QCOM Venus NV12 format image.
> 
> Review of attachment 8482481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The rest is OK.
> 
> ::: content/media/encoder/TrackEncoder.cpp
> @@ +197,5 @@
> >      while (!iter.IsEnded()) {
> >        VideoChunk chunk = *iter;
> >        if (!chunk.IsNull()) {
> >          gfx::IntSize imgsize = chunk.mFrame.GetImage()->GetSize();
> >          gfxIntSize intrinsicSize = chunk.mFrame.GetIntrinsicSize();
> 
> Let's keep this check for now.
> 
> This Venus format is a form of PLANAR_YCBCR (right?) so keeping this check
> shouldn't cause problems.

 No, HW decoder outputs GrallocImage so the format will be GRALLOC_PLANAR_YCBCR. So in order to support MP4 transcoding this check needs to be removed (that is, undoing bug 1000736).

 I'll add more code in OMXCodecWrapper::Encode() to check invalid input and make sure invalid & unsupported input image will return error.

Comment 35

[John Lin [:jhlin][:jolin]]

Attachment: Support encoding QCOM proprietary Venus NV12 images.

Validate input image before sending to OMX codec.

Comment 36

[John Lin [:jhlin][:jolin]]

Attachment: Catch errors from OMXCodecWrapper and pass them to recorder.

Instead of reporting error, just drop the frame when OMX codec is out of input buffer.

Comment 37

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8487703 [details] [diff] [review]
Support encoding  QCOM proprietary Venus NV12 images.

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

::: content/media/encoder/TrackEncoder.cpp
@@ -199,5 @@
>        if (!chunk.IsNull()) {
>          gfx::IntSize imgsize = chunk.mFrame.GetImage()->GetSize();
>          gfxIntSize intrinsicSize = chunk.mFrame.GetIntrinsicSize();
> -#ifdef MOZ_WIDGET_GONK
> -        // Block the video frames come from video source.

Is it possible to receive others image format on non-qualcomm platform?

Comment 38

[John Lin [:jhlin][:jolin]]

(In reply to Randy Lin [:rlin] from comment #37)
> Comment on attachment 8487703 [details] [diff] [review]
> Support encoding  QCOM proprietary Venus NV12 images.
> 
> Review of attachment 8487703 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/TrackEncoder.cpp
> @@ -199,5 @@
> >        if (!chunk.IsNull()) {
> >          gfx::IntSize imgsize = chunk.mFrame.GetImage()->GetSize();
> >          gfxIntSize intrinsicSize = chunk.mFrame.GetIntrinsicSize();
> > -#ifdef MOZ_WIDGET_GONK
> > -        // Block the video frames come from video source.
> 
> Is it possible to receive others image format on non-qualcomm platform?

Yes. SPRD (Dolphin) H.264 decoder output is HAL_PIXEL_FORMAT_YCbCr_420_SP. With this patch and attachment 8487705 [details] [diff] [review], encoder will report error and stop recorder for that case.

Comment 39

[John Lin [:jhlin][:jolin]]

Attachment: Support encoding QCOM proprietary Venus NV12 images.

Carry forward r+ and update commit message.

Comment 40

[John Lin [:jhlin][:jolin]]

Attachment: Catch errors from OMXCodecWrapper and pass them to recorder.

Carry forward r+ and update commit message.

Comment 41

[John Lin [:jhlin][:jolin]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=29fe10f7d1fd

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da60e75ad339
https://hg.mozilla.org/integration/mozilla-inbound/rev/88d0ae140cc1

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da60e75ad339
https://hg.mozilla.org/mozilla-central/rev/88d0ae140cc1

Comment 44

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2:

This issue does NOT reproduce on Flame 2.2:

Flame 2.2 

Device: Flame 2.2 Master KK (319mb) (Full Flash)
Build ID: 20141015040201
Gaia: 5f1f0960ae9d22acf2a324ad37a48174d6df87f6
Gecko: 62f0b771583c
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 36.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

The video records and plays back properly.
----------------------------------------------------

This issue still occurs on Flame 2.1 and 2.0. Filed a new bug 1083413