- hr = IWICPersistStream_LoadEx(persist, This->parent->stream, NULL, persist_options);
- if (FAILED(hr))
- ERR("IWICPersistStream_LoadEx error %#x\n", hr);
We can't use that stream because we provide separate seek and read/write methods to libtiff, and libtiff may rely on the stream's position to not change between operations.
This illustrates a design flaw in WIC. Normally, we would just clone the stream, but IWICStream doesn't implement the Clone method. I think we are probably supposed to create a new IWICStream based on the given stream, but the resulting stream won't be thread-safe with ours. I think for now it's OK to re-use this stream as long as you seek it back to where it was when you're done, but we may have to change that later.
Also, LoadEx should (but does not yet) save the stream so it can create an IWICFastMetadataEncoder. If you re-use the bitmap decoder's stream, you should also use the WICPersistOptionNoCacheStream flag when you create the metadata reader to make sure it won't do that.
Your GetReaderByIndex method should check whether This->metadata_reader has been set and return an error if not.
Otherwise, the patch looks good to me.
Vincent Povirk madewokherd@gmail.com wrote:
- hr = IWICPersistStream_LoadEx(persist, This->parent->stream, NULL, persist_options);
- if (FAILED(hr))
- ERR("IWICPersistStream_LoadEx error %#x\n", hr);
We can't use that stream because we provide separate seek and read/write methods to libtiff, and libtiff may rely on the stream's position to not change between operations.
libtiff doesn't rely on stream positions, and my use of stream matches MSDN guidelines on writing a WIC enabled codec.
This illustrates a design flaw in WIC. Normally, we would just clone the stream, but IWICStream doesn't implement the Clone method. I think we are probably supposed to create a new IWICStream based on the given stream, but the resulting stream won't be thread-safe with ours. I think for now it's OK to re-use this stream as long as you seek it back to where it was when you're done, but we may have to change that later.
There is no need to seek the stream back.
Also, LoadEx should (but does not yet) save the stream so it can create an IWICFastMetadataEncoder. If you re-use the bitmap decoder's stream, you should also use the WICPersistOptionNoCacheStream flag when you create the metadata reader to make sure it won't do that.
Your GetReaderByIndex method should check whether This->metadata_reader has been set and return an error if not.
No, that's not an error. TIFF always provides metadata since IFD is the core of TIFF. If a metadata reader can't be created it's OK to return 0 metadata items.
- hr = IWICPersistStream_LoadEx(persist, This->parent->stream, NULL, persist_options);
- if (FAILED(hr))
- ERR("IWICPersistStream_LoadEx error %#x\n", hr);
We can't use that stream because we provide separate seek and read/write methods to libtiff, and libtiff may rely on the stream's position to not change between operations.
libtiff doesn't rely on stream positions, and my use of stream matches MSDN guidelines on writing a WIC enabled codec.
We should provide the stream interface to libtiff as documented and not rely on implementation details.
Even without libtiff, you will end up using the same stream for multiple metadata handlers, each of which could end up using it later, unless you specify WICPersistOptionNoCacheStream.
Your GetReaderByIndex method should check whether This->metadata_reader has been set and return an error if not.
No, that's not an error. TIFF always provides metadata since IFD is the core of TIFF. If a metadata reader can't be created it's OK to return 0 metadata items.
That's fine, but your check in GetCount suggests that having no metadata reader is a valid state, and GetReaderByIndex(0) will crash in that case. If it's not valid, you shouldn't need to test for it in GetCount.
Also, I just noticed that you're calling create_metadata_reader unconditionally from QueryInterface, and not checking in the process whether the reader has already been created. That will leak if QI is called multiple times.
I'm not sure doing real work inside QueryInterface is a good idea. It's pretty surprising.
I think it would make more sense to do this when a method on IWICMetadataBlockReader is first called, or in Initialize if WICDecodeMetadataCacheOnLoad is specified (though doing it in Initialize would require creating the frame objects then as well, and probably combining their refcounts with the parent).
Vincent Povirk madewokherd@gmail.com wrote:
We can't use that stream because we provide separate seek and read/write methods to libtiff, and libtiff may rely on the stream's position to not change between operations.
libtiff doesn't rely on stream positions, and my use of stream matches MSDN guidelines on writing a WIC enabled codec.
We should provide the stream interface to libtiff as documented and not rely on implementation details.
Even without libtiff, you will end up using the same stream for multiple metadata handlers, each of which could end up using it later, unless you specify WICPersistOptionNoCacheStream.
That's not the case of an IFD metadata reader, it creates all the items on load, so creating multiple metadata readers from the same stream won't hurt.
Your GetReaderByIndex method should check whether This->metadata_reader has been set and return an error if not.
No, that's not an error. TIFF always provides metadata since IFD is the core of TIFF. If a metadata reader can't be created it's OK to return 0 metadata items.
That's fine, but your check in GetCount suggests that having no metadata reader is a valid state, and GetReaderByIndex(0) will crash in that case. If it's not valid, you shouldn't need to test for it in GetCount.
That's a valid point, sorry, I misread your comment first time.
Also, I just noticed that you're calling create_metadata_reader unconditionally from QueryInterface, and not checking in the process whether the reader has already been created. That will leak if QI is called multiple times.
I'm not sure doing real work inside QueryInterface is a good idea. It's pretty surprising.
I think it would make more sense to do this when a method on IWICMetadataBlockReader is first called, or in Initialize if WICDecodeMetadataCacheOnLoad is specified (though doing it in Initialize would require creating the frame objects then as well, and probably combining their refcounts with the parent).
MSDN suggests different ways of creating a metadata reader: 1. when creating a IWICBitmapFrameDecode 2. when creating a block reader 3. inside of GetReaderByIndex
It's all up to an implementor. I decided to that at #1. Each instance of IWICBitmapFrameDecode has its own metadata reader, I don't see how it could be created twice or leaked.
Dmitry Timoshkov dmitry@baikal.ru wrote:
Also, I just noticed that you're calling create_metadata_reader unconditionally from QueryInterface, and not checking in the process whether the reader has already been created. That will leak if QI is called multiple times.
I'm not sure doing real work inside QueryInterface is a good idea. It's pretty surprising.
I think it would make more sense to do this when a method on IWICMetadataBlockReader is first called, or in Initialize if WICDecodeMetadataCacheOnLoad is specified (though doing it in Initialize would require creating the frame objects then as well, and probably combining their refcounts with the parent).
MSDN suggests different ways of creating a metadata reader:
- when creating a IWICBitmapFrameDecode
- when creating a block reader
- inside of GetReaderByIndex
It's all up to an implementor. I decided to that at #1. Each instance of IWICBitmapFrameDecode has its own metadata reader, I don't see how it could be created twice or leaked.
I take that back, thanks for the comments!
-
Dmitry Timoshkov -
Vincent Povirk