Last Comment Bug 686742 - Move YUV conversion code into gfxUtil functions
: Move YUV conversion code into gfxUtil functions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla10
Assigned To: Oleg Romashin (:romaxa)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 598868
  Show dependency treegraph
 
Reported: 2011-09-14 12:32 PDT by Oleg Romashin (:romaxa)
Modified: 2011-09-29 09:18 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move yuv2rgb conversion into gfxUtils.. (15.17 KB, patch)
2011-09-14 12:35 PDT, Oleg Romashin (:romaxa)
tterribe: review-
Details | Diff | Splinter Review
move yuv2rgb conversion into gfxUtils.. (14.72 KB, patch)
2011-09-24 12:00 PDT, Oleg Romashin (:romaxa)
tterribe: review+
Details | Diff | Splinter Review
Fixed comment, for CHECKIN (14.72 KB, patch)
2011-09-26 10:36 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
move yuv2rgb conversion into gfxUtils.. (15.31 KB, patch)
2011-09-26 22:12 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
move yuv2rgb conversion into gfxUtils. to Push (15.39 KB, patch)
2011-09-28 12:16 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2011-09-14 12:32:32 PDT
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 1 Oleg Romashin (:romaxa) 2011-09-14 12:35:47 PDT
Created attachment 560224 [details] [diff] [review]
move yuv2rgb conversion into gfxUtils..
Comment 2 Timothy B. Terriberry (:derf) 2011-09-19 16:18:45 PDT
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.
Comment 3 Oleg Romashin (:romaxa) 2011-09-24 12:00:34 PDT
Created attachment 562249 [details] [diff] [review]
move yuv2rgb conversion into gfxUtils..

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 4 Timothy B. Terriberry (:derf) 2011-09-26 10:14:53 PDT
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.
Comment 5 Oleg Romashin (:romaxa) 2011-09-26 10:36:24 PDT
Created attachment 562474 [details] [diff] [review]
Fixed comment, for CHECKIN
Comment 6 Oleg Romashin (:romaxa) 2011-09-26 22:12:34 PDT
Created attachment 562660 [details] [diff] [review]
move yuv2rgb conversion into gfxUtils..

Fixed Mac bustage, moved n nsCoreAnimationSupport out from ImageLayers.h, and replaced with class declaration
Comment 7 Oleg Romashin (:romaxa) 2011-09-26 22:13:08 PDT
try build
https://tbpl.mozilla.org/?tree=Try&rev=f8695b89cbc6
Comment 8 Oleg Romashin (:romaxa) 2011-09-28 12:16:51 PDT
Created attachment 563140 [details] [diff] [review]
move yuv2rgb conversion into gfxUtils. to Push

Fixed some macosx related build problems
https://tbpl.mozilla.org/?tree=Try&rev=3c11a4c84212
Comment 10 Michael Wu [:mwu] 2011-09-29 01:31:48 PDT
https://hg.mozilla.org/mozilla-central/rev/2a443d5f2944

Note You need to log in before you can comment on or make changes to this bug.