Hayoung,

Here are my comments:

1) You need to set your tab settings in Visual Studio
   as shown in the attached image VS7TabSettings.png.

2) datatype/image/png/filewriter/Umakefil

   -
   -project.ExportFunction("RMACreateInstance",
   -                       "IUnknown** ppObj",
   -                       "common/include",
   -                       "hxcom.h")

   RMACreateInstance entrypoint should not be removed. It
   is necessary for the PNG file writer to be loaded as
   a Helix plugin.

3) ffdriver.h

   +#define RESIZINGTHUMBNAILWIDTH_OPTION_NAME	"ResizingThumbnailWidth"
   +#define RESIZINGTHUMBNAILHEIGHT_OPTION_NAME	"ResizingThumbnailHeight"

   We should not call these anything to do with "thumbnail" since
   this decoding might be used with general video transcoding.
   So let's call these options as follows:

   +#define DECODED_VIDEO_RESIZE_WIDTH_OPTION_NAME  "DecodedVideoResizeWidth"
   +#define DECODED_VIDEO_RESIZE_HEIGHT_OPTION_NAME "DecodedVideoResizeHeight"

3) ffdriver.cpp

+	if (SUCCEEDED(retVal))
+	{
+		ULONG32 ulResizingThumbnailWidth = 0;
+		if (SUCCEEDED(m_pOptions->GetPropertyULONG32(RESIZINGTHUMBNAILWIDTH_OPTION_NAME, ulResizingThumbnailWidth)))
+		{
+			retVal = pNewProps->SetPropertyULONG32(RESIZINGTHUMBNAILWIDTH_OPTION_NAME, ulResizingThumbnailWidth);
+		}
+	}	
+	if (SUCCEEDED(retVal))
+	{
+		ULONG32 ulResizingThumbnailHeight = 0;
+		if (SUCCEEDED(m_pOptions->GetPropertyULONG32(RESIZINGTHUMBNAILHEIGHT_OPTION_NAME, ulResizingThumbnailHeight)))
+		{
+			retVal = pNewProps->SetPropertyULONG32(RESIZINGTHUMBNAILHEIGHT_OPTION_NAME, ulResizingThumbnailHeight);
+		}
+	}	

    There is no need for these options to be passed on to the file writer, since the
    resizing is done at the video decoder level.

4) main.cpp

  +#define RESIZINGTHUMBNAILWIDTH_OPTION_STRING			"RTW"
  +#define RESIZINGTHUMBNAILHEIGHT_OPTION_STRING		"RTH"

  In order to match the new option name, then let's call
  these command-line option strings as follows:
  
  +#define DECODED_VIDEO_RESIZE_WIDTH_OPTION_STRING  "DVRW"
  +#define DECODED_VIDEO_RESIZE_HEIGHT_OPTION_STRING "DVRH"

5) pngwrtr.cpp

+	, m_uIBitsPerPixel(0)
+	, m_uIResizingThumbnailWidth(0)
+	, m_uIResizingThumbnailHeight(0)

   Please use the prefix "m_ul" instead of "m_uI" on these variables.

+	HX_RELEASE(m_pColorFuncAccessor);

  I'm not sure how this even compiles, since the ColorFuncAccess class does not
  have a Release() method. This should be HX_DELETE() instead.

+
+	ULONG32 resizingThumbnailWidth = 0;
+	retVal = m_pProperties->GetPropertyULONG32("ResizingThumbnailWidth", resizingThumbnailWidth);
+	if (SUCCEEDED(retVal))
+	{
+		m_uIResizingThumbnailWidth = resizingThumbnailWidth;
+	}	
+
+	ULONG32 resizingThumbnailHeight = 0;
+	retVal = m_pProperties->GetPropertyULONG32("ResizingThumbnailHeight", resizingThumbnailHeight);
+	if (SUCCEEDED(retVal))
+	{
+		m_uIResizingThumbnailHeight = resizingThumbnailHeight;
+	}
+
+	if (SUCCEEDED(retVal))
+	{
+		ULONG32 ulColorFormatOut = 0;
+		if (SUCCEEDED(m_pOptions->GetPropertyULONG32(COLORCONVERT_OPTION_NAME, ulColorFormatOut)))
+		{
+			retVal = pNewProps->SetPropertyULONG32(COLORCONVERT_OPTION_NAME, ulColorFormatOut);
+		}
+	}

     The PNG file writer does not need to resize the video. Resizing will
     be done at the video decoder. Also, the PNG file writer always knows
     to color convert the video to RGB. It does not need the "ColorConvert"
     option. In fact, the "ColorConvert" option should be independent of
     the PNG file writer - it is intended for the color conversion that
     happens immediately after the video decode.

-        ulPropertyValue = // m_FileHeaderInfo.m_bFullyNative ? 1 : 0;
+        //ulPropertyValue = // m_FileHeaderInfo.m_bFullyNative ? 1 : 0;

    If this code is not needed (and you know it will never be needed
    in the future), then remove it.

+	else if (strcasecmp("video/X-HX-I420", pMimeTypeData) == 0)
+	{		
+		m_bDoConversion		= TRUE;
+		m_ulColorFormatIn	= CID_I420;	
+		
+		bRetVal = SUCCEEDED(m_pProperties->GetPropertyULONG32("ColorConvert", m_ulColorFormatOut));
+		if (bRetVal == FALSE)
+		{
+			// Default setting as RGB24
+			m_ulColorFormatOut = CID_RGB24;

   Color conversion in the PNG file writer does not depend
   on the presence of the "ColorConvert" option. PNG *always*
   needs RGB. Therefore, if the input color format is not
   either RGB24 or RGB32, then PNG always knows to color
   convert to an RGB format. The "ColorConvert" option tells
   the system whether or not to color convert immediately
   after the video decode.

6) pngwrtr.h - some changes will be required here in order
   to make the changes I've suggested above in pngwrtr.cpp.

7) vdecoder.cpp - You have removed the color conversion here.
   That is not what we want. We want to leave the color conversion
   capability in vdecoder.cpp, which is still controlled by the
   "ColorConvert" option. We want to *add* color conversion
   capability in the PNG file writer which is NOT controlled
   by the "ColorConvert" option. Since the PNG file writer knows
   that it needs RGB, then it knows that it will need to 
   color convert from whatever input color format to RGB.

+		// check to see if ProcessDecodedTimeUnits reach the value,
+		// if so, call OnStreamDone function to stop getting any more packets.
+		if (m_bProcessDecodedTimeUnits)
+		{
+			if (m_ulProcessDecodedTimeUnits == 0) 
+			{
+				OnStreamDone(HXR_OK, pPacket->GetStreamNumber());
+				m_bProcessDecodedTimeUnits = FALSE;
+			}
+			else
+			{
+				m_ulProcessDecodedTimeUnits--;
+			}			
+		}
+

  - I see that the code above calls StreamDone() when you have
    passed on m_ulProcessDecodedTimeUnits packets. However, I don't
    see any code that prevents us from decoding any packets
    we get *after* we have called StreamDone(). We definitely
    need that.

  - We also need to add the ability here to resize based
    upon the "DecodedVideoResizeWidth" and "DecodedVideoResizeHeight"
    options. We may also want to add a "DecodedVideoResizePreserveAspect"
    option (which defaults to 1) which will allow the user to preserve
    the aspect ratio when resizing is done.

    If no color conversion is being done, then we can use
    the resizing functions in datatype/common/util/pub/resize.h to
    resize the video frame. If color conversion is being done, then
    it's possible that we might be able to do the resizing and
    the color conversion in one step, since some color converters
    support that. However, some color converters only support a limited
    resizing (like resizing to width/2 and height/2). So the logic
    can work something like this (in pseudocode):

     if (doing color conversion)
     { 
         if (doing resizing)
         {
             // Try resizing and color conversion at the same time
             retVal = ColorConvert()
             if (FAILED(retVal))
             {
                 // Resize Y plane using resize.h functions
                 // Resize U plane using resize.h functions
                 // Resize V plane using resize.h functions

                 // Now do color conversion with no resizing
                 retVal = ColorConvert()
             }
         }
         else
         {
              // Color convert as usual
         }
     }
     else if (doing resizing)
     {
         // Resize Y plane using resize.h functions
         // Resize U plane using resize.h functions
         // Resize V plane using resize.h functions
     }

8) vdecoder.h

   As I said above in the comments for vdecoder.cpp, we do NOT
   want to remove the color conversion capability from
   vdecoder.cpp.

Please rework these changes and re-submit the CR.

Eric

=======================================
Eric Hyche (ehyche at real.com)
Principal Engineer
RealNetworks, Inc.


>-----Original Message-----
>From: datatype-dev-bounces at helixcommunity.org [mailto:datatype-dev-bounces at helixcommunity.org] On
>Behalf Of ???(Hayoung Choi)
>Sent: Thursday, September 25, 2008 3:13 PM
>To: datatype-dev at lists.helixcommunity.org
>Subject: [datatype-dev] CR: Thumbnail generation improvement updates
>
>Modified by: Hayoung Choi (hayoung.choi at ap.real.com)
>
>Date: September 25, 2008
>
>Project: Thumbnail generation improvement updates
>
>
>
>Synopsis:
>
>=============
>
>PNG file writer supports both RGB24/RGB32 thumbnail generations, resizing of thumbnail image.
>
>The color conversion code is moved into the PNG file writer from video decoder.
>
>The option “ProcessDecodedTimeUnits” is added into dtdrive application.
>
>
>
>Overview:
>
>=============
>
>1) RGB24/RGB32 thumbnails support and moving color conversion into PNG file writer
>
>The color conversion codes are moved into PNG file writer from video decoder.
>
>So, if the mime type of video source is RGB, color conversion will not be happened, but in case of
>I420 conversion will be happened PNG file writer inside.
>
>The default value of “bits per pixel” for RGB is 24, or you can choose RGB24/RGB32 with the
>application option (-CC).
>
>
>
>dtdrive.exe +u +f -DV 1 -CC RGB32 -MOF 5 -W videotest.png videotest.rm
>
>
>
>2) Adding “ProcessDecodedTimeUnits”
>
>If the count of decoded frames reach the value of “ProcessDecodedTimeUnits” which was set by the
>application option (-PDTU), the video decoder calls OnStreamDone().
>
>Then decoding will not occurred any more.
>
>
>
> dtdrive.exe +u +f -DV 1 -PDTU 5 -CC RGB32 -MOF 5 -W videotest.png videotest.rm
>
>
>
>3) Resizing the thumbnail image for PNG
>
>Resizing thumbnail image frame option is added. You can resize thumbnail with the option
>ResizingThumbnailWidth (-RTW) / ResizingThumbnailHeight (-RTH).
>
>
>
>dtdrive.exe +u +f -DV 1 -MOF 5 -RTW 160 -RTH 90 -W videotest.png videotest.rm
>
>
>
>Files Modified:
>
>=============
>
>[File 1] - datatype/image/png/filewriter/Umakefil
>
>[File 2] - datatype/image/png/filewriter/pngwrtr.cpp
>
>[File 3] - datatype/image/png/filewriter/pngwrtr.h
>
>[File 4] - datatype/tools/dtdriver/apps/dtdrive/main.cpp
>
>[File 5] - datatype/tools/dtdriver/engine/ffdriver.cpp
>
>[File 6] - datatype/tools/dtdriver/engine/pub/ffdriver.h
>
>[File 7] - datatype/tools/dtdriver/decoder/video/vdecoder.cpp
>
>[File 8] - datatype/tools/dtdriver/decoder/video/vdecoder.h
>
>
>
>=============
>
>Image Size and Heap Use impact (Client -Only):
>
>NA
>
>
>
>
>
>=============
>
>Platforms and Profiles Affected:
>
>x86 Windows XP professional SP3 (Intel Core(TM)2 T5600 CPU 1.83GHz)
>
>
>
>
>
>=============
>
>Distribution Libraries Affected:
>
>pngwrtr.dll
>
>dtdrive.lib
>
>dtdrengine.lib
>
>dtdrviddec.lib
>
>
>
>=============
>
>Distribution library impact and planned action:
>
>NA
>
>
>
>=============
>
>Platforms and Profiles Build Verified:
>
>System id: win32-i386-vc7
>
>x86 Windows XP professional SP3 (Intel Core(TM)2 T5600 CPU 1.83GHz)
>
>Microsoft Compiler 13.10.6030
>
>
>
>
>
>=============
>
>Platforms and Profiles Functionality verified:
>
>x86 Windows XP professional SP3 (Intel Core(TM)2 T5600 CPU 1.83GHz)
>
>
>
>Branch: HEAD and Atlas310
>
>Target: dtdrive
>
>Profile: helix-client-all-defines
>
>
>
>
>
>=============
>
>Copyright assignment:
>
>I am a RealNetworks employee or contractor
>
>
>
>=============
>
>QA Instructions:
>
>NA

-------------- next part --------------
A non-text attachment was scrubbed...
Name: VS7TabSettings.png
Type: image/png
Size: 36819 bytes
Desc: not available
Url : http://lists.helixcommunity.org/pipermail/datatype-dev/attachments/20080926/c9643019/VS7TabSettings-0001.png