969312 - [MediaEncoder] Support data from GonkCameraSource in HW video recording path.

Status: RESOLVED WONTFIX

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

Description

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

Android video recorder and the recorder of mozCamera use a "metadata" format that both HW encoder and camera understand. In their implementation, the effort of buffer copying is very small (memcpy() 8 bytes on Nexus 4).

  In current Gecko GetUserMedia() implementation on B2G, the camera input data is from preview window. Since HW encoder doesn't necessary support its pixel format, full frame color conversion/copying is needed.

  For B2G HW video encoding, getting camera input data from GonkCameraSource seems a better choice.

Comment 1

[u459114]

This is for video recording performance improvement.
1. Connect camera recording callback for media recording flow
2. Keep camera preview callback for media playback(<video> tag)
We need to be able to carry different video chunk in media track for different track listener, media recorder and <video>

I think this one should be a blocker for 1.5

Comment 2

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

(In reply to C.J. Ku[:CJKu] from comment #1)
> This is for video recording performance improvement.
> 1. Connect camera recording callback for media recording flow
> 2. Keep camera preview callback for media playback(<video> tag)
> We need to be able to carry different video chunk in media track for
> different track listener, media recorder and <video>
> 
> I think this one should be a blocker for 1.5

Current idea is:
1. Refactoring - move most of B2G camera related code out from MediaEngineWebRTC/MediaEngineWebRTCVideoSource into its own file
2. Add new data source (GonkCameraSource)
3. Create new chunk data type for GonkCameraSource (new gfx::layers::Image subclass)
4. Make MediaRecorder/MediaEncoder aware of and receive data from new source
5. Let OMXVideoEncoder accept new data type in 3, and still handle muted/black screen data correctly

Looks like 1~3 can be tracked with Bug 938034, so change the summary of this bug to address 4 & 5.

Comment 3

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

Attachment: Part 1: [WIP] Make MediaEncoder aware of data from GonkCameraSource.

Make MediaEncoder a direct listener of source media stream, and accepts video data only from direct listener callback.

Comment 4

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

Attachment: Part 2: [WIP] Accept video data from GonkCameraSource in OMXVideoEncoder.

Enable 'metadata in buffer' mode to eliminate full size buffer copying.

Comment 5

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

Still missing:
1. Notification of recording start/stop.
2. Black metadata buffer for muted frame.

Comment 6

[u459114]

Comment on attachment 8381191 [details] [diff] [review]
Part 2: [WIP] Accept video data from GonkCameraSource in OMXVideoEncoder.

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

add peter

Comment 7

[u459114]

Comment on attachment 8381190 [details] [diff] [review]
Part 1: [WIP] Make MediaEncoder aware of data from GonkCameraSource.

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

::: content/media/encoder/MediaEncoder.h
@@ +84,5 @@
>  
>    /**
> +   * Notified when AppendToTrack() of source media stream is called. It's used
> +   * to receive data from Camera.
> +   */

#ifdef MOZ_B2G_CAMERA

Comment 8

[u459114]

Comment on attachment 8381191 [details] [diff] [review]
Part 2: [WIP] Accept video data from GonkCameraSource in OMXVideoEncoder.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +263,4 @@
>    if (!aImage) {
> +#ifdef MOZ_B2G_CAMERA
> +    inBuf->setRange(0, 0);
> +    dstSize = 0;

Is this code design for blank image while mediarecoder::pause?

@@ +337,5 @@
> +        IntSize imgSize = img->GetSize();
> +        CODEC_ERROR("metadata buffer:%p data:%p len:%d time:%lld size:%dx%d flags:%x",
> +          buffer, src, len, aTimestamp, imgSize.width, imgSize.height, aInputFlags);
> +        inBuf->setRange(0, len);
> +        memcpy(dst, src, len);

Only META data copy? there should be no image copy here

Comment 9

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

Comment on attachment 8381190 [details] [diff] [review]
Part 1: [WIP] Make MediaEncoder aware of data from GonkCameraSource.

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

::: content/media/encoder/MediaEncoder.h
@@ +85,5 @@
>    /**
> +   * Notified when AppendToTrack() of source media stream is called. It's used
> +   * to receive data from Camera.
> +   */
> +  virtual void NotifyRealtimeData(MediaStreamGraph* aGraph, TrackID aID,

Also need flag add on notifier(MSG).

Comment 10

[Peter Chang[:pchang]]

Comment on attachment 8381191 [details] [diff] [review]
Part 2: [WIP] Accept video data from GonkCameraSource in OMXVideoEncoder.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +263,4 @@
>    if (!aImage) {
> +#ifdef MOZ_B2G_CAMERA
> +    inBuf->setRange(0, 0);
> +    dstSize = 0;

Suggest to add TODO here for blank image insertion for b2g

@@ +282,5 @@
>      MOZ_ASSERT(aWidth == img->GetSize().width &&
>                 aHeight == img->GetSize().height);
>  
> +    if (format != ImageFormat::B2G_METADATA_BUFFER_CAMERA) {
> +      inBuf->setRange(0, yLen + uvLen);

I would prefer to move setRange to GRALLOC_PLANAR_YCBCR and PLANAR_YCBCR

@@ +326,5 @@
>                               dst);
> +#ifdef MOZ_B2G_CAMERA
> +    } else if (format == ImageFormat::B2G_METADATA_BUFFER_CAMERA) {
> +      MediaBuffer* buffer = static_cast<MediaBuffer*>(img->GetImplData());
> +      if (img->IsSentToCompositor() || !buffer) {

I saw img->MarkSent() called in line 345, you add isSentToCompositor() call here.
Does it mean it is possible to encode the same image twice?
If so, why do we have same image for encoding twice?

@@ +337,5 @@
> +        IntSize imgSize = img->GetSize();
> +        CODEC_ERROR("metadata buffer:%p data:%p len:%d time:%lld size:%dx%d flags:%x",
> +          buffer, src, len, aTimestamp, imgSize.width, imgSize.height, aInputFlags);
> +        inBuf->setRange(0, len);
> +        memcpy(dst, src, len);

Please add comment here to address only meta data(buffer) copy here.

Comment 11

[Ivan Tsay (:ITsay)]

Not a blocker for H.264 hardware support for webRTC in v1.5

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Per-talk with John Lin, steal this bug from him.

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 1: [WIP] Make MediaEncoder aware of data from GonkCameraSource. v2.0

Rebased and carry cjku and rlin's f+.

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 2: [WIP] Accept video data from GonkCameraSource in OMXVideoEncoder. v2.0

Rebased and carry cjku and rlin's f+.

Comment 15

[Maire Reavy [:mreavy]]

Alfredo - Are you planning on finishing these patches?  

You might also look at the changes to the getUserMedia capture code expected in the webrtc.org 43 update, which is about to land.  See bug 1198458; there's a patch queue you can apply to see the updates.  There's code to support texture backing of captured data, I believe.
Thanks!

Comment 16

[Alfredo Yang (:alfredo)]

(In reply to Maire Reavy [:mreavy] (Back Dec 21st - ni?:mreavy) from comment #15)
> Alfredo - Are you planning on finishing these patches?  
> 
> You might also look at the changes to the getUserMedia capture code expected
> in the webrtc.org 43 update, which is about to land.  See bug 1198458;
> there's a patch queue you can apply to see the updates.  There's code to
> support texture backing of captured data, I believe.
> Thanks!

Thanks for info!
I'm no longer working on it. Munro may have interest to complete it.

Comment 17

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Closing as we are not working on Firefox OS anymore.