Page MenuHomePhabricator

bitdepth of UploadStashFile is wrong
Closed, ResolvedPublic

Description

using prop=stashimageinfo with siiprop=bitdepth gives bitdepth="0", not the 8 which is stored in the database.

You have to override the stub File::getBitDepth.


Version: 1.20.x
Severity: normal

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:10 AM
bzimport set Reference to bz34952.
bzimport added a subscriber: Unknown Object (MLST).

Yes, but the upload module will already provide it to you on the result, but you can also see it of stashimageinfo.

Hello, I'm new here, and I'd like to work on the bug, can anyone indicate where the getBitDepth() function needs to be overridden?

See the inheritance diagram of File. This logical place to add getBitDepth would be UnregisteredLocalFile.

In the longer term, I suppose LocalFile and UploadStashFile should inherit this functionality from a commons parent. But that's not an easy bug anymore.

I can't seem to find where the bit depth is being stored for an UnregisteredLocalFile. I don't seem to be able to find it in the database, which is what the implementation of LocalFile seems to do.

Can somebody point me to where I can find the stored bit depth or does it have to be calculated?

After speaking to @MarkTraceur on IRC, it seems like the only way to do this directly at the moment is to make a call to ImageHandler::getImageSize() which doesn't seem very elegant because it skips the middle levels of abstraction. If someone has a better solution, please do comment. Meanwhile, attempting a patch that makes a direct call.

Change 181782 had a related patch set uploaded (by Polybuildr):
UnregisteredLocalFile.php: Override File::getBitDepth() stub

https://gerrit.wikimedia.org/r/181782

Patch-For-Review

I seem to have not noticed UnregisteredLocalFile::getImageSize() earlier. Returned the bits field of that method in the patch.

Also, UnregisteredLocalFile::getImageSize() seems to want a $filename parameter which it doesn't use. Will submit a patch later to remove that.

In T36952#854515, @Tgr wrote:

See the inheritance diagram of File. This logical place to add getBitDepth would be UnregisteredLocalFile.

That was bad advice, sorry; the uploadstash table already stores the bitdepth so the functionality can be more efficiently implemented in UploadStashFile. (The patch is still a valid improvement for UnregisteredLocalFile, though.)

While the efficiency is a valid argument, the UploadStashFile class has the comment "Arguably UnregisteredLocalFile should be handling its own file repo but that class is a bit retarded currently".

I could just as easily submit a patch for adding a getBitDepth method in UploadStashFile, but no similar functionality seems to exist in the same class. getImageSize, etc. are instead implemented in UnregisteredLocalFile.

What do you think the appropriate way to proceed would be?

I think UploadStashFile should have the same logic as LocalFile (except for the memcached layer): have private properties for all database fields, have a load() method which initializes them from the database, and the getters should just call load() and then return the property.

Probably the UnregisteredLocalFile-based version would work just fine as users can only access their own stash and no popular tool uses the stash API to retrieve metadata, so it would not cause too much load, but it's still best to not leave the possibility open.

@Tgr, that makes sense. Would that be necessary to just fix this issue, though? My patch takes care of the immediate issue (albeit superficially). Rewriting the class could maybe be a separate task?

@Tgr, uploaded a new patch set with an updated commit message that mentions the need for some larger restructuring. Please do review it and let me know if it's okay as a quick fix.

Could someone please review the relevant commit on Gerrit? @Tgr, @MarkTraceur?

Change 181782 merged by jenkins-bot:
UnregisteredLocalFile.php: Override File::getBitDepth() stub

https://gerrit.wikimedia.org/r/181782

@Tgr, I'm slightly confused about the restructuring of these classes. UploadStashFile has a comment that says "Arguably UnregisteredLocalFile should be handling its own file repo but that class is a bit retarded currently". However, as we discussed, this particular bug should probably have been fixed in UploadStashFile (by changing its behaviour) instead of UnregisteredLocalFile. Could you please explain how these classes should probably be behaving?

In my opinion (but I'm probably not the best person to give advice on this) UnregisteredLocalFile is for files for which we don't have any additional information, just the file itself; all logic that does not make use of a database should go there. UploadStashFile has additional metadata (an owner, in the future probably file description and such) and should mainly deal with that. But the existence of an upload stash database table also means that some of the parent functionality can be made more efficient by using the DB for caching. Whether that's actually worthwile, I'm not sure.

I'm planning to do this restructuring (will create a new task) and am looking for some help on the details.

@MarkTraceur, any comments about the restructuring @Tgr talked about? Is there someone who should be CCed in this task to give a better perspective?

I'm the wrong person to comment, I have faith in @Tgr but maybe @aaron could comment too.