I'm going to use YUV conversion code in MediaBridge (direct video rendering) and want to use that code outside of BasicImage code... Suggesting to move that into separate gfx util functions

Comment on attachment 560224 [details] [diff] [review] move yuv2rgb conversion into gfxUtils.. Review of attachment 560224 [details] [diff] [review]: ----------------------------------------------------------------- r-, see inline comments. ::: gfx/thebes/gfxUtils.cpp @@ +520,5 @@ > PRInt32(aIn.Width()), PRInt32(aIn.Height())); > return gfxRect(aOut->x, aOut->y, aOut->width, aOut->height).IsEqualEdges(aIn); > } > > +bool You don't use this return value, ConvertYUVtoRGB (the only reasonable consumer of this functions output) doesn't need it, and it adds work to compute it. I'd suggest just getting rid of it. @@ +521,5 @@ > return gfxRect(aOut->x, aOut->y, aOut->width, aOut->height).IsEqualEdges(aIn); > } > > +bool > +gfxUtils::GetOptimalDestFormat(const PlanarYCbCrImage::Data& aData, The function name here implies that it only modifies aSuggestedFormat, when in fact it also modifies aSuggestedSize. Perhaps GetOptimalDestFormatAndSize()? I'm also not wild about the word "Optimal"... a number of the decisions in this function are required for _correctness_, and have nothing to do with speed. Maybe GetYCbCrToRGBDestFormatAndSize()? That both a) tells you that this is related to YCbCrToRGB (otherwise the only clue is the PlanarYCbCrImage::Data parameter), and b) doesn't imply that this step is optional. @@ +537,5 @@ > + // YCbCr to RGB conversion rather than on the RGB data when rendered. > + PRBool prescale = aSuggestedSize.width > 0 && aSuggestedSize.height > 0 && > + aSuggestedSize != aData.mPicSize; > + > + gfxIntSize size(prescale ? aSuggestedSize.width : aData.mPicSize.width, This gets used exactly once, in the IsScaleYCbCrToRGB565Fast check, where it's known that prescale is true. You should just use aSuggestedSize directly. @@ +543,5 @@ > + > + if (aSuggestedFormat == gfxASurface::ImageFormatRGB16_565) { > +#if defined(HAVE_YCBCR_TO_RGB565) > + if (prescale && > + !gfx::IsScaleYCbCrToRGB565Fast(aData.mPicX, There are additional restrictions on the buffer size and alignment required to use this function. See the NS_ASSERTION's at the beginning of ScaleYCbCrToRGB565() for details. If you're planning to use this function outside the BasicPlanarYCbCrImage context, you may need to explicitly check these conditions and fall back to the non-prescaled case. IsScaleYCbCrToRGB565Fast() can't currently do it because it doesn't have access to the pointers and strides (it was meant to check things that could be checked once, in advance, but the current delay in setting mScaleHint makes that harder in BasicImages). @@ +575,5 @@ > + See bugs 639415 and 640073. */ > + if (aData.mPicX != 0 || aData.mPicY != 0 || yuvtype == gfx::YV24) > + prescale = PR_FALSE; > + } > + gfxIntSize destSize(prescale ? aSuggestedSize.width : aData.mPicSize.width, If you get rid of the "modified" return value, you can replace the entire rest of the function with if (!prescale) { aSuggestedSize = aData.mPicSize; } @@ +586,5 @@ > + return modified; > +} > + > +bool > +gfxUtils::ConvertYUVtoRGB(const PlanarYCbCrImage::Data& aData, I'd prefer ConvertYCbCrToRGB instead of YUV (to match the functions it calls). @@ +597,5 @@ > + aData.mYSize.height, > + aData.mCbCrSize.width, > + aData.mCbCrSize.height); > + > + long int stride = You shouldn't recompute this. That creates an implicit contract with the caller about the buffer layout that a) no one will expect and b) you haven't documented. Instead, you should just take aStride as a parameter along with aDestBuffer. @@ +665,5 @@ > + aData.mCbCrStride, > + stride, > + yuvtype); > + } > + return true; It's not clear what possible, useful return value we could have here, and you're not checking it anyway. Suggest just returning void. ::: gfx/thebes/gfxUtils.h @@ +46,5 @@ > class gfxDrawable; > class nsIntRegion; > struct nsIntRect; > > +using namespace mozilla::layers; I don't think a "using namespace" clause is a good idea in a header file. Please don't do it. @@ +127,5 @@ > */ > static gfxFloat ClampToScaleFactor(gfxFloat aVal); > + > + /** > + * Helper function for ConvertYUVtoRGB that helps tofind optimal RGB buffer size and format s/helps tofind/finds the/ @@ +129,5 @@ > + > + /** > + * Helper function for ConvertYUVtoRGB that helps tofind optimal RGB buffer size and format > + * for given YCbCrImage. > + * @param aSuggestedFormat, aSuggestedSize can be modified a) This isn't proper doxygen (one @param per parameter). b) You should document _how_ these things get modified, i.e., "aSuggestedFormat will be set to ImageFormatRGB24 if the desired format is not supported." and "aSuggestedSize will be set to the picture size from aData if either the suggested size was {0,0} or simultaneous scaling and conversion is not supported." This makes it clear what cases the caller has to handle if they don't get what they asked for. @@ +138,5 @@ > + gfxASurface::gfxImageFormat& aSuggestedFormat, > + gfxIntSize& aSuggestedSize); > + > + /** > + * Convert YCbCrImage into RGB aDestBuffer You should mention that the parameters here _must_ have been passed to GetOptimalDestFormat (or whatever you rename that function to) before attempting to do the conversion.

No, I'm not planning to use it outside of BasicPlannarImage, this is needed in order to allocate destination data outside of Players protocol, such like MediaBridge.

Comment on attachment 562249 [details] [diff] [review] move yuv2rgb conversion into gfxUtils.. Review of attachment 562249 [details] [diff] [review]: ----------------------------------------------------------------- r+ with one small change. ::: gfx/thebes/gfxUtils.cpp @@ +580,5 @@ > +gfxUtils::ConvertYCbCrToRGB(const PlanarYCbCrImage::Data& aData, > + const gfxASurface::gfxImageFormat& aDestFormat, > + const gfxIntSize& aDestSize, > + unsigned char* aDestBuffer, > + long int aStride) The value you're passing to aStride is a PRInt32, and the corresponding argument to the YCbCr conversion functions is an int. This parameter should be one of those two types (I don't strongly care which one), not a long int.

Fixed Mac bustage, moved n nsCoreAnimationSupport out from ImageLayers.h, and replaced with class declaration

try build https://tbpl.mozilla.org/?tree=Try&rev=f8695b89cbc6

Fixed some macosx related build problems https://tbpl.mozilla.org/?tree=Try&rev=3c11a4c84212