Project

General

Profile

Bug #1038

QuickTimeVideo::discard() works incorrectly with MemIo

Added by Anonymous over 6 years ago. Updated about 5 years ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
video
Target version:
0.28
Start date:
08 Mar 2015
Due date:
% Done:

0%

Estimated time:

Description

Hi!
There's a bug in libexiv2 which stops KDE's GwenView from parsing mp4 video metadata correctly. GwenView's idea is to use libexiv2 on first 65536 bytes of the file, using MemIo.
It's probably an incorrect idea for mp4 becausemoov may be in the end of file, not in the beginning, but it still works this way...
So, when the QuickTimeVideo parser wants to skip some data by discard()ing it, it calls MemIo::seek(). But MemIo::seek() discovers that the requested offset is beyond the end of the currently allocated area (65536 bytes) and just fails (returns 1) without changing current position. discard() does not change for the error code and also returns.
So, the block data remains non-skipped and decodeBlock() starts to parse data as new blocks, which is incorrect and usually leads to a hang.
The possible ways of fixing this problem are either to allow MemIo::seek() to seek beyond the EOF, or teaching discard() to look at error codes.

Files

patch-exiv2-0.24-fix-mp4.diff (2.88 KB) patch-exiv2-0.24-fix-mp4.diff Anonymous, 08 Mar 2015 22:58
2009_10_20_10_32_27.avi (35.4 KB) 2009_10_20_10_32_27.avi Bad truncated avi file Anonymous, 15 Mar 2015 08:30
patch-exiv2-0.24-fix-mp4.diff (3.17 KB) patch-exiv2-0.24-fix-mp4.diff Updated patch Anonymous, 01 Apr 2015 09:35

Associated revisions

Revision 3620 (diff)
Added by Robin Mills over 6 years ago

#1038. Thank You to Vitaliy for reporting this and providing the patch.

Revision 3628 (diff)
Added by Robin Mills over 6 years ago

#1038. Initialize timeScale_ in ctor because MSVC (correctly) refused header member variable initialization.

History

#1

Updated by Anonymous over 6 years ago

discard() does not change for the error code and also returns.

sorry, I mean discard() does not check for the error code and also returns.

#2

Updated by Anonymous over 6 years ago

I fixed this issue + another issue with timescale==0 with the attached patch

#3

Updated by Robin Mills over 6 years ago

Patch subitted: r3620. Thank You to Vitaliy for reporting this and providing the patch.

Vitaliy
I had to edit your patch and use the utility 'patch' to submit this because my SVN client (SmartSVN) didn't recognise it. And the file quicktimevideo.hpp has been moved from src -> include/exiv2. I hope you will help us another day and I would like to ask you to prepare the patch in the following steps:

  1. $ svn://dev.exiv2.org/svn/trunk
  2. -- edit the files --
  3. (optional build and test) $ make config ; ./configure ; make -j4 ; sudo make install ; make -j4 samples ; make tests
  4. $ svn diff > bla-dee-bla.patch

When I submit your patch it will be built and tested on Mac / Linux / Cygwin / MinGW / MSVC on our Jenkins build server at http://exiv2.dyndns.org:8080

I will add a topic "Submitting Patches" to our FAQ on exiv2.org

Thank You for doing this, Vitaliy. Your contribution to Exiv2 is appreciated.

#4

Updated by Anonymous over 6 years ago

By the way, am I correct about the default value of 1 for the time scale?

#5

Updated by Robin Mills over 6 years ago

  • Category set to video
  • Status changed from New to Assigned
  • Assignee set to Abhinav Badola
  • Target version set to 0.25

Vitaly

The value 1 seems OK to me. I'll assign this issue to Abhinav, one of our video engineers, and ask him to review this.

I have added a topic to our FAQ about submitting patches. http://dev.exiv2.org/projects/exiv2/wiki/Guidelines_for_submitting_patches

Abhinav
If you're happy, can you mark this issue "Resolved". If you're not certain, assign the issue to me and I will give this more careful thought.

#6

Updated by Anonymous over 6 years ago

OK, thanks for quick response :)

#7

Updated by Abhinav Badola over 6 years ago

Vitaliy Filippov wrote:

OK, thanks for quick response :)

Hi Vitaliy,

Thanks for finding the issue and providing the patch. :)

I was under the impression that KDE apps use a wrapper library libkexiv2 to access Exiv2 libraries, and not use it directly.
And similarly the Gnome family uses the wrapper libgexiv2.

You found a very good bug, and this use-case never occurred to me, prior to your bug report.

Actually, as you have very well described in your bug, we can never rely on the initial metadata found in Quicktime types of files.

This is because the metadata can be placed anywhere, and thus, sometimes towards the end of the file as well.
In a lot of samples that I used for testing, I found that the metadata was hidden towards the end of file.
Maybe Gwenview should also upgrade, if it doesn't want to miss any information. (But that is all up to their team members)

But, indeed, it is a very relevant bug and I am happy that you found it in time. :)
Thanks again, for the valuable patch.

@Robin,
Thanks for adding the patch to Exiv2.
I will look into the code soon.
I will mark this as resolved, if I feel that the timescale patch works fine with all the videos samples that I have. :)

#8

Updated by Anonymous over 6 years ago

In a lot of samples that I used for testing, I found that the metadata was hidden towards the end of file.

Metadata in the end is actually the default layout for mp4 files... I think it comes from the simplicity of encoding process: encoder does not know full metadata until the encoding is finished, so it writes metadata only after finishing the file. This makes a problem for video pseudo-streaming over http: you can't start watching file until it's fully downloaded. So there's a 'qt-faststart' utility which moves metadata ('moov' atom) from the end to the beginning...

#9

Updated by Anonymous over 6 years ago

Reported the bug to KDE bugtracker: https://bugs.kde.org/show_bug.cgi?id=345169

+++ It seems your RiffVideo parser also has a similar bug - libexiv2 hangs reading a truncated AVI file... see the attachment. You can reproduce the bug using commandline exiv2 utility.

#10

Updated by Anonymous over 6 years ago

I've discovered that there is another method that should be patched, it's MemIo::getb() - if it's not, exiv2 crashes on truncated JPEG files.

See the updated patch in attachment.

#11

Updated by Robin Mills over 6 years ago

  • Assignee changed from Abhinav Badola to Thomas Beutlich

Thanks for reporting this, Vitaliy. I'm on vacation at the moment. So I'm going to ask Thomas to have a look at your patch as he's involved in other work concerning MemIO.

Thomas:
Can you have a look at this patch, please? If you don't have time, please assign it to me and I'll deal with it next week. When we're done with this, push the bug back to Abhinav.

Robin

#12

Updated by Thomas Beutlich over 6 years ago

  • Assignee changed from Thomas Beutlich to Abhinav Badola

The MemIo::getb patch was commited by r3677.

I wonder why only MemIo::seek was patched by r3620 but not e.g. RemoteIo::seek. Is this really by design or simply overlooked?

#13

Updated by Anonymous over 6 years ago

I just haven't looked at other Io's in my patch, if that's what you're speaking about :)

#14

Updated by Thomas Beutlich over 6 years ago

Vitaliy Filippov wrote:

I just haven't looked at other Io's in my patch, if that's what you're speaking about :)

Yes, that is what I had in mind.

#15

Updated by Alan Pater over 6 years ago

  • Target version changed from 0.25 to 0.26
#16

Updated by Robin Mills about 5 years ago

  • Status changed from Assigned to New
  • Assignee deleted (Abhinav Badola)
  • Target version changed from 0.26 to 0.28

I'm going to defer this for v0.27. I'm also removing Abhinav as the assignee. I hope to have a team hangout in October 2016 to deal with assignments for v0.27.

Also available in: Atom PDF