MediaConverter does not handle errors gracefully when sniffing files (easy)

[yourpalal]

MediaFileInfo::LoadInfo() ignores many return codes and can end up with a GIGO situation. Some examples are:

*should check track->InitCheck() after getting the track from TrackAt()

(MediaFileInfo.cpp:37)

*should check return of track->EncodedFormat()

(MediaFileInfo.cpp:39)

*should check return of track->DecodedFormat()

(MediaFileInfo.cpp:43)

*should check return of track->GetCodecInfo()

(MediaFileInfo.cpp:46)

There are similar problems when sniffing audio tracks (MediaFileInfo.cpp:58-88)

Once those are fixed, LoadInfo() should also return a status_t, which should be checked in MediaFileInfoView::Update().

[stippi]

Patch looks good, thanks. Some suggestions:

• When the passed file is NULL, return B_BAD_VALUE instead of B_ERROR.
• Initialize ret with B_OK instead of 0.
• Check for != B_OK instead of < B_OK. Technically the error codes are negative, but we have switched to prefer the direct check for B_OK in new code. (And old code where we come across it.)

Thanks a lot for your work! Coding style is fine, too.

[Barrett]

updated patch for #6497

[Barrett]

Thanks for your patience before all, the updated patch follow your suggestions. I'm thinking to start help the project fixing some bugs appropriate for my knowledge...can i work on #6595 also?

[korli]

Replying to Barrett:

thinking to start help the project fixing some bugs appropriate for my knowledge...can i work on #6595 also?

Sure, just post a comment on it.

[stippi]

Thanks for the patch! Applied in hrev39157.

[Barrett]

My changes added some issues so i prepared a patch that solve them. For example if in the list were one file and you remove it, the app displayed (wrongly) a BAlert. I also solved a TODO and improved the initial check for media files, indeed before my modifies the app passed every file to every plugin to see if there is one that support the format, for 10 files it requires more time (20 sec), Now it is more fast : )

[stippi]

The new patch looks mostly fine. The only problem is the _IsMediaFile(). In principle it's a good idea, but there are media file MIME types other than video/* and audio/*, for example application/ogg, but there are others. MediaPlayer does something similar, but it also looks for its own supported types for a fall back. At least the user can then force MediaPlayer to consider a file as supported regardless. In the case of MediaConverter, I would keep the old mechanism of trying to parse the file regardless of type. I don't know where you saw the delay... if this happens when the files are added to the list, it could be a solution to delay the sniffing to until the file is converted. What do you think?

[Barrett]

Sorry for delay and thanks for your reply. Yes the delay happens when the files are added to the list. Personally i prefer to check if a file is supported before to add it in the list, makes more sense for me. Delaying the Sniff, would signify to add temporarily a possibly unsupported file, maybe i'm wrong but (IMHO) it's not better from the user experience side, i want to be noticed soon if a file is not supported. Just to know...why the MediaPlayer approach is not good for MediaConverter?

[Barrett]

So...any suggestion? I want to improve the patch...

[stippi]

Hm, how about this suggestion: You could fill the list in a background (or the BApplication) thread. Check each file as before (by trying to load it as BMediaFile). That gives visual feedback to the user and makes sure that all files that can be loaded really are. Perhaps skip the BMediaFile construction for audio/* and video/* types to speed things up?

[Barrett]

I've looked at the "problem". The speed problems were a set of coincidences (haiku under vm, bad tests), sorry. So i have simply removed the media file check, it's ok i think.

[stippi]

Patch applied in hrev39853. To give me the satisfaction to be able to close a ticket, please don't attach new patches to already closed tickets, although in this case it was probably ok, it being a follow up patch. Still, technically, there were new bugs, which deserved a new ticket. :-)

[Barrett]

Thanks! I will ensure that next time there will not be a second patch : )