Closed Bug 965440 Opened 7 years ago Closed 7 years ago

VideoData::Create never actually creates a GRALLOC_PLANAR_YCBCR image

Categories

(Core :: Audio/Video, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bjacob, Assigned: sotaro)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attachment #8367502 - Flags: review?(chris.double)
Depends on: 964442
Attachment #8367502 - Flags: review?(chris.double) → review+
bjacob, do you have a plan to fix the problem? If you are busy, I could take a bug.
Flags: needinfo?(bjacob)
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.
Flags: needinfo?(bjacob)
Un-bitrot.
Attachment #8367502 - Attachment is obsolete: true
(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.
Assignee: bjacob → sotaro.ikeda.g
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
Current New texture does not support implicit fallback from pmem to shmem.
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
Status: NEW → ASSIGNED
Implement fallback from gralloc to shmem.
Attachment #8372449 - Attachment is obsolete: true
Fix mac build error.
Attachment #8373533 - Attachment is obsolete: true
Attachment #8373566 - Flags: review?(nical.bugzilla)
Attachment #8373566 - Flags: review?(chris.double)
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.
Attachment #8373566 - Flags: review?(nical.bugzilla) → review+
Attachment #8373566 - Flags: review?(chris.double) → review+
https://hg.mozilla.org/mozilla-central/rev/a4a8f80f280c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 979394
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?
Flags: needinfo?(sotaro.ikeda.g)
Saw in the other bug that we're going with the fix forward solution here.
Flags: needinfo?(sotaro.ikeda.g)
(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.
You need to log in before you can comment on or make changes to this bug.