Closed Bug 964442 Opened 6 years ago Closed 6 years ago

CreateImage() is weird API


(Core :: Graphics: Layers, defect)

Not set





(Reporter: jrmuizel, Assigned: bjacob)



(Whiteboard: [qa-])


(1 file, 1 obsolete file)

CreateImage takes an array of formats and creates and image with first one to succeed. Nearly all of the callers pass a single format. The only caller that I could find that did not passes an array where the second value is never looked at:

We should just a single format passed to CreateImage.
Blocks: 962784
There is one nontrivial hunk in this patch, fixing what we had identified as a bug:

-    ImageFormat format[2] = {ImageFormat::PLANAR_YCBCR, ImageFormat::GRALLOC_PLANAR_YCBCR};
     if (IsYV12Format(Y, Cb, Cr)) {
-      v->mImage = aContainer->CreateImage(format, 2);
-    } else {
-      v->mImage = aContainer->CreateImage(format, 1);
+      v->mImage = aContainer->CreateImage(ImageFormat::GRALLOC_PLANAR_YCBCR);
+    }
+    if (!v->mImage) {
+      v->mImage = aContainer->CreateImage(ImageFormat::PLANAR_YCBCR);

The existing code was introduced in bug 761018 and it seems unintentional that it effectively never used GRALLOC_PLANAR_YCBCR, as it always tried first PLANAR_YCBCR. It seems that the intent was to try GRALLOC_PLANAR_YCBCR if possible, where 'possible' is determined by the return value of IsYV12Format. So I took the liberty of fixing this in the same patch, as trying to imitate the old behavior bug-for-bug would have required a weird construct. In addition, I don't suppose that we really want to try GRALLOC outside of b2g (where it will just fail) so I added #ifdef so we only try it on B2G. I could be convinced that we don't want these ifdefs.
This version preserves bug-for-bug the old behavior, meaning that we don't try to use GRALLOC_PLANAR_YCBCR in VideoData::Create, as the existing code effectively never uses it.

This patch is written so as to minimize the amount of churn, whence the #if 0 's.

Now filing the bug to switch to the apparently intended behavior.
Attachment #8367476 - Attachment is obsolete: true
Attachment #8367476 - Flags: review?(jmuizelaar)
Attachment #8367495 - Flags: review?(jmuizelaar)
Blocks: 965440
Attachment #8367495 - Flags: review?(jmuizelaar) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.