Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#6497 closed bug (fixed)

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

Reported by: yourpalal Owned by: stippi
Priority: normal Milestone: R1
Component: Applications/MediaConverter Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description (last modified by 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().

Attachments (2)

mediac.diff (3.7 KB ) - added by Barrett 9 years ago.
updated patch for #6497
mediaconv2.diff (4.2 KB ) - added by Barrett 9 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by yourpalal, 9 years ago

Component: ApplicationsApplications/MediaConverter

comment:2 by yourpalal, 9 years ago

Description: modified (diff)

comment:3 by Barrett, 9 years ago

Has a Patch: set

comment:4 by stippi, 9 years ago

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.

by Barrett, 9 years ago

Attachment: mediac.diff added

updated patch for #6497

comment:5 by Barrett, 9 years ago

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?

in reply to:  5 comment:6 by korli, 9 years ago

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.

comment:7 by stippi, 9 years ago

Owner: changed from yourpalal to stippi
Status: newin-progress

comment:8 by stippi, 9 years ago

Thanks for the patch! Applied in hrev39157.

comment:9 by anevilyak, 9 years ago

Resolution: fixed
Status: in-progressclosed

comment:10 by Barrett, 9 years ago

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 : )

comment:11 by stippi, 9 years ago

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?

comment:12 by Barrett, 9 years ago

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?

comment:13 by Barrett, 9 years ago

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

comment:14 by stippi, 9 years ago

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?

comment:15 by Barrett, 9 years ago

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.

by Barrett, 9 years ago

Attachment: mediaconv2.diff added

comment:16 by stippi, 9 years ago

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. :-)

comment:17 by Barrett, 9 years ago

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

Note: See TracTickets for help on using tickets.