Re: [patch 1/1] dlls/windowscodecs/pngformat.c: implemented 'PngDecoder_Block_GetCount'
On 10/5/2012 06:43, max+git(a)mtew.isa-geek.net wrote:
From: Max TenEyck Woodbury <max+git(a)mtew.isa-geek.net>
--- dlls/windowscodecs/pngformat.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/dlls/windowscodecs/pngformat.c b/dlls/windowscodecs/pngformat.c index 686f9c6..e8e7cbe 100644 --- a/dlls/windowscodecs/pngformat.c +++ b/dlls/windowscodecs/pngformat.c @@ -899,8 +899,10 @@ static HRESULT WINAPI PngDecoder_Block_GetContainerFormat(IWICMetadataBlockReade static HRESULT WINAPI PngDecoder_Block_GetCount(IWICMetadataBlockReader *iface, UINT *pcCount) { - FIXME("%p,%p: stub\n", iface, pcCount); - return E_NOTIMPL; + PngDecoder *This = impl_from_IWICMetadataBlockReader(iface); + if (!pcCount) return E_INVALIDARG; + *pcCount = This->ref; + return S_OK; }
static HRESULT WINAPI PngDecoder_Block_GetReaderByIndex(IWICMetadataBlockReader *iface, Return reference count as block count?
On 10/05/2012 03:55 AM, Nikolay Sivov wrote:
On 10/5/2012 06:43, max+git(a)mtew.isa-geek.net wrote:
From: Max TenEyck Woodbury <max+git(a)mtew.isa-geek.net>
--- dlls/windowscodecs/pngformat.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/dlls/windowscodecs/pngformat.c b/dlls/windowscodecs/pngformat.c index 686f9c6..e8e7cbe 100644 --- a/dlls/windowscodecs/pngformat.c +++ b/dlls/windowscodecs/pngformat.c @@ -899,8 +899,10 @@ static HRESULT WINAPI PngDecoder_Block_GetContainerFormat(IWICMetadataBlockReade static HRESULT WINAPI PngDecoder_Block_GetCount(IWICMetadataBlockReader *iface, UINT *pcCount) { - FIXME("%p,%p: stub\n", iface, pcCount); - return E_NOTIMPL; + PngDecoder *This = impl_from_IWICMetadataBlockReader(iface); + if (!pcCount) return E_INVALIDARG; + *pcCount = This->ref; + return S_OK; } static HRESULT WINAPI PngDecoder_Block_GetReaderByIndex(IWICMetadataBlockReader *iface, Return reference count as block count?
I've screwed up. This is NOT what is supposed to be returned here. I am in the process of digging out what this really should be. Please do NOT apply this patch! (However, it does trigger at least one app to do something useful and reveals the need for other changes...)
Hmm. I definitely misunderstood what this function was intended to do. As I understand things now, there are two ways to approach processing graphical information: 1) As a stream of information to be processed in the order it arrives. 2) As an aggregate to be processed all at once. The PNG format is specifically designed to allow it to be processed as a stream with good efficiency. Processing the aggregate is done by processing the stream and accumulating the aggregated information. The key interface to the stream approach is to enumerate the blocks and process each one as you come to it. This approach has already been implemented. The strategy of the aggregate approach is to have an array of blocks and use an index to access the elements of the array. The key elements are getting the number of blocks (this function) and getting access to the block by its index number (using 'GetReaderByIndex'). This approach has not been implemented. To do this efficiently, it should be possible to validate the correctness of each block WITHOUT getting into any more details about the blocks than possible. Have I got this right so far?
You should take a look at the PNG format spec, particularly the part about chunks: http://www.libpng.org/pub/png/spec/1.2/PNG-Structure.html I believe what's needed here is to return all ancillary chunks.
On 10/05/2012 10:58 AM, Vincent Povirk wrote:
You should take a look at the PNG format spec, particularly the part about chunks: http://www.libpng.org/pub/png/spec/1.2/PNG-Structure.html
I believe what's needed here is to return all ancillary chunks.
First, thank you for the pointer to the HTML version of the PNG spec. I had the PDF one already, but the cross references make it a lot easier to use. Beyond that, I am not sure what you are talking about. The value to be returned (at the address specified by the 2nd parameter) is an unsigned integer. There is no place to return 'chunks'; only a place to return a count. In order to get that count, each and every one of the chunks needs to be scanned and validated. During the scanning process, it would make sense to store the location of chunks in an indexed data structure. That would nominally be an array, but a linked list or tree would also work. For a general WIC design, there needs to be at least three layers with indexes built into at least two of the layers. The top layer is the entire file. The next layer is the set of frames in the file. PNG only allows one frame per file, but other formats can have several (even many) frames. The next layer is the set of blocks (in this case "chunks") in each frame. In some cases there might be a need for additional layers. All this means I need to dig out the 'class' structure of IWIC, but it would help if someone can confirm that I have not gone off the deep end yet.
Beyond that, I am not sure what you are talking about. The value to be returned (at the address specified by the 2nd parameter) is an unsigned integer. There is no place to return 'chunks'; only a place to return a count.
Other methods on that interface allow you to return an IWICMetadataReader, which need to be based on individual PNG chunks. Obviously, the GetCount method just gives the number that can be returned.
In order to get that count, each and every one of the chunks needs to be scanned and validated. During the scanning process, it would make sense to store the location of chunks in an indexed data structure. That would nominally be an array, but a linked list or tree would also work.
I don't think it's really necessary to verify the checksum. You just need to read a few bytes of header to get the chunk type and the location of the next one. See the png_read_chunk function. I don't understand anything you said after that paragraph.
On 10/05/2012 05:53 PM, Vincent Povirk wrote:
Beyond that, I am not sure what you are talking about. The value to be returned (at the address specified by the 2nd parameter) is an unsigned integer. There is no place to return 'chunks'; only a place to return a count.
Other methods on that interface allow you to return an IWICMetadataReader, which need to be based on individual PNG chunks. Obviously, the GetCount method just gives the number that can be returned.
Agreed then. This is only one part of the total aggregate interface. If this routine is implemented, the other methods need to be done as well. The enumeration of the chunks needs to be recorded.
In order to get that count, each and every one of the chunks needs to be scanned and validated. During the scanning process, it would make sense to store the location of chunks in an indexed data structure. That would nominally be an array, but a linked list or tree would also work.
I don't think it's really necessary to verify the checksum. You just need to read a few bytes of header to get the chunk type and the location of the next one. See the png_read_chunk function.
I think the CRC check would be a good idea. As the rational in the spec mentioned, doing it guards against transition (and other) errors. <cynic_mode>No assurance that Murphy (or his so unkind worshipers) can't get in of course.</cynic_mode>
I don't understand anything you said after that paragraph.
I was thinking about other formats the IWIC framework has to handle. Not just about PNG.
participants (3)
-
Max TenEyck Woodbury -
Nikolay Sivov -
Vincent Povirk