965440 - VideoData::Create never actually creates a GRALLOC_PLANAR_YCBCR image

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Try GRALLOC_PLANAR_YCBCR in VideoData::Create on B2G

This code in VideoData::Create :

http://hg.mozilla.org/mozilla-central/file/b28e216c8d07/content/media/MediaDecoderReader.cpp#l224

    // Currently our decoder only knows how to output to PLANAR_YCBCR
    // format.
    ImageFormat format[2] = {PLANAR_YCBCR, GRALLOC_PLANAR_YCBCR};
    if (IsYV12Format(Y, Cb, Cr)) {
      v->mImage = aContainer->CreateImage(format, 2);
    } else {
      v->mImage = aContainer->CreateImage(format, 1);
    }

was introduced by the patch in bug 761018. Here, CreateImage takes an array of formats, iterates over it, and uses the first format that succeeds. So here, it always tries PLANAR_YCBCR before trying GRALLOC_PLANAR_YCBCR, meaning that it will virtually never use GRALLOC_PLANAR_YCBCR.

Bug 964442 is changing CreateImage to no longer take an array of formats, and instead take a plain aFormat parameter. As part of that, this code is being rewritten as follows to preserve the existing behavior or always using PLANAR_YCBCR:

    v->mImage = aContainer->CreateImage(ImageFormat::PLANAR_YCBCR);

Now, it seems that the original intention was to use GRALLOC_PLANAR_YCBCR when possible, and there is even a helper function in that file, IsYV12Format(), that is only used to determine whether that is possible.

Attaching a patch that switches behavior to trying GRALLOC_PLANAR_YCBCR on B2G only when IsYV12Format() returns true.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6094047ffca

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/026d41e6bf55

https://tbpl.mozilla.org/php/getParsedLog.php?id=33894433&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33894161&tree=Mozilla-Inbound

Comment 3

[Sotaro Ikeda [:sotaro]]

bjacob, do you have a plan to fix the problem? If you are busy, I could take a bug.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

The only plan is to reproduce this locally on emulator. I with emulator-jb, and ran into the problem that it is nontrivial to run tests on emulator-jb (bug 965542). There is a trick explained there, which I was planning on trying as soon as I get a chance. But I am a little bit busy at the moment, so I can't tell when I will be able to try. You are very welcome to take this bug, if you want.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Try GRALLOC_PLANAR_YCBCR in VideoData::Create on gonk

Un-bitrot.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Benoit Jacob [:bjacob] from comment #4)
> But I am a little bit busy at the moment,
> so I can't tell when I will be able to try. You are very welcome to take
> this bug, if you want.

Thanks, I am going to take the bug.

Comment 7

[Sotaro Ikeda [:sotaro]]

From the following crash log, it seems to fail to allocate gralloc buffer.

>     INFO -  Thread 26 (crashed)
>     INFO -   0  libxul.so!mozilla::layers::GrallocBufferActor::GetFrom(mozilla::layers::SurfaceDescriptorGralloc const&) [StrongPointer.h : 128 + 0x0]
>     INFO -       r4 = 0x44ad4ad4    r5 = 0x44ad4b34    r6 = 0x44ad4a34    r7 = 0x00000000
>     INFO -       r8 = 0x44ad4a68    r9 = 0x436d4540   r10 = 0x43b06a70    fp = 0x00000000
>     INFO -       sp = 0x44ad4a10    lr = 0x408e09d7    pc = 0x408d9092
>     INFO -      Found by: given as instruction pointer in context
>     INFO -   1  libxul.so!mozilla::layers::GrallocImage::SetData(mozilla::layers::PlanarYCbCrData const&) [GrallocImages.cpp:35c57af24bf5 : 101 + 0x7]
>     INFO -       r4 = 0x43b069d0    r5 = 0x44ad4b34    r6 = 0x44ad4a34    r7 = 0x00000000
>     INFO -       r8 = 0x44ad4a68    r9 = 0x436d4540   r10 = 0x43b06a70    fp = 0x00000000
>     INFO -       sp = 0x44ad4a18    pc = 0x408e09d7
>     INFO -      Found by: call frame info
>     INFO -   2  libxul.so!mozilla::VideoData::Create(mozilla::VideoInfo&, mozilla::layers::ImageContainer*, mozilla::layers::Image*, long long, long long, long long, mozilla::VideoData::YCbCrBuffer const&, bool, long long, nsIntRect) [MediaDecoderReader.cpp:35c57af24bf5 : 268 + 0x5]
>     INFO -       r4 = 0x44ad4c70    r5 = 0x44ad4bf8    r6 = 0x432a7520    r7 = 0x43b069d0
>     INFO -       r8 = 0x436d2780    r9 = 0x00000001   r10 = 0x00000001    fp = 0x00000000
>     INFO -       sp = 0x44ad4b08    pc = 0x40e0df89
>     INFO -      Found by: call frame info

Comment 8

[Sotaro Ikeda [:sotaro]]

Current New texture does not support implicit fallback from pmem to shmem.

Comment 9

[Sotaro Ikeda [:sotaro]]

GrallocImage::SetData() does not check if gralloc buffers allocation failure/success. Even if it fails, GrallocImage::SetData() is going to get gralloc buffer from SurfaceDescriptor.
 http://mxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Try GRALLOC_PLANAR_YCBCR in VideoData::Create on gonk

Implement fallback from gralloc to shmem.

Comment 11

[Sotaro Ikeda [:sotaro]]

try 
https://tbpl.mozilla.org/?tree=Try&rev=9cbf30a2bf64

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Try GRALLOC_PLANAR_YCBCR in VideoData::Create on gonk

Fix mac build error.

Comment 13

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=e74a4b4ba1e5

Comment 14

[Nicolas Silva [:nical]]

Comment on attachment 8373566 [details] [diff] [review]
patch v4 - Try GRALLOC_PLANAR_YCBCR in VideoData::Create on gonk

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

I am not an expert on the MediaData stuff so I let most of this review for Chris. The small GrallocImages change is sane.

Comment 15

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a4a8f80f280c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4a8f80f280c

Comment 17

[Jason Smith [:jsmith]]

This probably should be backed out. The regressing bug here shows that video thumbnails in the video app are broken by this patch.

Sotaro - What do you think?

Comment 18

[Jason Smith [:jsmith]]

Saw in the other bug that we're going with the fix forward solution here.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Jason Smith [:jsmith] from comment #17)
> This probably should be backed out. The regressing bug here shows that video
> thumbnails in the video app are broken by this patch.
> 
> Sotaro - What do you think?

I already created a fix in Bug 979394. It seems better to progress the fix at Bug 979394.