Closed Bug 964442 Opened 6 years ago Closed 6 years ago

CreateImage() is weird API

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jrmuizel, Assigned: bjacob)

References

Details

(Whiteboard: [qa-])

Attachments

(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:

http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderReader.cpp#226

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};
+#ifdef MOZ_WIDGET_GONK
     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);
+    }
+#endif
+    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+
https://hg.mozilla.org/mozilla-central/rev/2cf4e419eb96
Status: NEW → RESOLVED
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.