Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(81789)

Issue 1524: Empty input files will not be processed in 'dump_video' (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 4 weeks ago by dan
Modified:
3 months, 1 week ago
Reviewers:
negge, derf, tdaede
Visibility:
Public.

Description

Empty input files made the program to crash and they will be skipped.
Same behaviour is implemented in 'player_example'.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M examples/dump_video.c View 1 chunk +5 lines, -1 line 3 comments Download

Messages

Total messages: 4
dan
4 months, 4 weeks ago #1
derf
The error message would be good to fix. The refactoring I suggest is probably a ...
4 months, 3 weeks ago #2
dan
On 2017/05/06 22:59:13, derf wrote: > The error message would be good to fix. The ...
4 months, 3 weeks ago #3
dan
4 months, 3 weeks ago #4
On 2017/05/07 08:08:00, dan wrote:
> On 2017/05/06 22:59:13, derf wrote:
> > The error message would be good to fix. The refactoring I suggest is
probably
> a
> > subject for a future patch.
> > 
> > http://review.xiph.org/1524/diff/2588/examples/dump_video.c
> > File examples/dump_video.c (right):
> > 
> > http://review.xiph.org/1524/diff/2588/examples/dump_video.c#newcode273
> > examples/dump_video.c:273: fprintf(stderr, "Invalid Ogg\n");
> > It would be nice if the error message here matched the one down on line 337.
> > 
> > http://review.xiph.org/1524/diff/2588/examples/dump_video.c#newcode343
> > examples/dump_video.c:343: if (daala_p) {
> > Really, this flag was supposed to record whether or not we successfully
found
> a
> > Daala stream, and we weren't supposed to try to use it unless we did.
> > 
> > http://review.xiph.org/1524/diff/2588/examples/dump_video.c#newcode362
> > examples/dump_video.c:362: pic_height = di.pic_height;
> > But I can see that that is no longer the case (is this the source of the
> > crash?). This should probably be refactored to mostly-eliminate the daala_p
> flag
> > and simply exit if we don't find a Daala stream (there isn't really anything
> > useful this program can do if we don't).
> > 
> > daala_p is mostly a holdover from the corresponding code in player_example,
> back
> > when player_example also supported multiplexed audio.
> 
> The source of segfault crash in 'dump_video' was that the header of the input
> file was corrupted and therefore extremely large values for width, height,
etc.
> was read and processed. The problem is that the header validation code in
> 'dump_video' is not as good as 'player_example'. In 'player_example' same
> corrupted file was identified while it was missed in 'dump_video'.
> 
> The reason that I used this error message is because 'player_example' uses
same
> error message for invalid files. I have changed the error message to what you
> suggested. Again, I had deleted my development branch by mistake, so I will
> submit a new review for the fixed patch. Sorry if there will be any trouble!
:)

The new patch has issue number 1526! Thanks for your review.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld