Bug in ExtractFrame API (extracts frames incorrectly)

Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames correctly.

Issue: This API was calculating the file_header and frame_header offset only for the first frame which is not the right logic. We need to skip the file and frame header every time we extract a new frame.

Also added a unit test case which compares the extracted frame with the frame stored in text file.

BUG=webrtc:6761
NOPRESUBMIT=true

Committed: https://crrev.com/b7636b4656d7f8c368963f2256dc2ef7b7ba89c8
Cr-Commit-Position: refs/heads/master@{#15260}

[charujain, 2016-11-24 13:45:04]

Description was changed from

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

BUG=webrtc:6761
==========

to

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

BUG=webrtc:6761
==========

[charujain, 2016-11-24 13:53:14]

Patchset #2 (id:20001) has been deleted

[charujain, 2016-11-25 08:50:17]

Patchset #2 (id:30001) has been deleted

[charujain, 2016-11-25 13:22:08]

Description was changed from

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

BUG=webrtc:6761
==========

to

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

Also added a unit test case which compares the extracted frame with the frame
stored in text file. 

BUG=webrtc:6761
==========

[charujain, 2016-11-25 13:22:27]

charujain@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[charujain, 2016-11-25 13:23:32]

PTAL.

[phoglund, 2016-11-25 14:39:08]

minor comments

https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):

https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:144: if (bytes_read !=
static_cast<size_t>(frame_size) && ferror(input_file)) {
I wonder if it should be or here? Are we happy if parts of frame_header is junk
even if ferror is happy?

https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right):

https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:52: uint8_t*
result_frame = new uint8_t[size];
These leak. I think you can allocate these guys on the stack. Else, use
scoped_ptr or delete[] them.

[phoglund, 2016-11-25 14:50:12]

Also: keep CL descriptions within 72 columns or
they look ugly it git log and the review tool.

[charujain, 2016-11-28 09:18:47]

On 2016/11/25 14:39:08, phoglund wrote:
> minor comments
> 
>
https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
> File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):
> 
>
https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
> webrtc/tools/frame_analyzer/video_quality_analysis.cc:144: if (bytes_read !=
> static_cast<size_t>(frame_size) && ferror(input_file)) {
> I wonder if it should be or here? Are we happy if parts of frame_header is
junk
> even if ferror is happy?

I could not think of a way to check here if the extracted frame is junk or not.
We cannot check for keyword like "FRAME" in the bytes
extracted since that can we valid string as well. This error checks for the last
frame or EOF. 

>
https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
> File webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2529923002/diff/100001/webrtc/tools/frame_analy...
> webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc:52: uint8_t*
> result_frame = new uint8_t[size];
> These leak. I think you can allocate these guys on the stack. Else, use
> scoped_ptr or delete[] them.

[phoglund, 2016-11-28 11:51:04]

https://codereview.webrtc.org/2529923002/diff/110001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):

https://codereview.webrtc.org/2529923002/diff/110001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:166: if
(ferror(input_file)) {
Discussed offline: it's better to check for EOF with feof() here.

[charujain, 2016-11-28 12:36:02]

On 2016/11/28 11:51:04, phoglund wrote:
>
https://codereview.webrtc.org/2529923002/diff/110001/webrtc/tools/frame_analy...
> File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):
> 
>
https://codereview.webrtc.org/2529923002/diff/110001/webrtc/tools/frame_analy...
> webrtc/tools/frame_analyzer/video_quality_analysis.cc:166: if
> (ferror(input_file)) {
> Discussed offline: it's better to check for EOF with feof() here.

Done. PTAL.

[phoglund, 2016-11-28 12:37:37]

lgtm w/ addition of one missing fclose().

https://codereview.webrtc.org/2529923002/diff/120001/webrtc/tools/frame_analy...
File webrtc/tools/frame_analyzer/video_quality_analysis.cc (right):

https://codereview.webrtc.org/2529923002/diff/120001/webrtc/tools/frame_analy...
webrtc/tools/frame_analyzer/video_quality_analysis.cc:164: if (feof(input_file))
{
Hmm, you need a fclose here though.

[charujain, 2016-11-28 12:44:51]

Description was changed from

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

Also added a unit test case which compares the extracted frame with the frame
stored in text file. 

BUG=webrtc:6761
==========

to

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

Also added a unit test case which compares the extracted frame with the frame
stored in text file. 

BUG=webrtc:6761

NOPRESUBMIT=true
NOTRY=true
==========

[charujain, 2016-11-28 12:45:48]

The CQ bit was checked by charujain@webrtc.org

[charujain, 2016-11-28 12:45:49]

The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2529923002/#ps130001 (title:
"Add missing fclose.")

[commit-bot: I haz the power, 2016-11-28 12:45:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-28 13:03:59]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1480337148514320,
"parent_rev": "3b15f6a0d46a2985c158271e619c6b72c1269330", "commit_rev":
"0e793911662a5040f21cc7601693f92855436e3e"}

[commit-bot: I haz the power, 2016-11-28 13:04:01]

Description was changed from

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

Also added a unit test case which compares the extracted frame with the frame
stored in text file. 

BUG=webrtc:6761

NOPRESUBMIT=true
NOTRY=true
==========

to

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

Also added a unit test case which compares the extracted frame with the frame
stored in text file. 

BUG=webrtc:6761

NOPRESUBMIT=true
NOTRY=true
==========

[commit-bot: I haz the power, 2016-11-28 13:04:02]

Committed patchset #9 (id:130001)

[commit-bot: I haz the power, 2016-11-28 13:04:11]

Description was changed from

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame. 

Also added a unit test case which compares the extracted frame with the frame
stored in text file. 

BUG=webrtc:6761

NOPRESUBMIT=true
NOTRY=true
==========

to

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame.

Also added a unit test case which compares the extracted frame with the frame
stored in text file.

BUG=webrtc:6761

NOPRESUBMIT=true
NOTRY=true

Committed: https://crrev.com/b7636b4656d7f8c368963f2256dc2ef7b7ba89c8
Cr-Commit-Position: refs/heads/master@{#15260}
==========

[commit-bot: I haz the power, 2016-11-28 13:04:12]

Patchset 9 (id:??) landed as
https://crrev.com/b7636b4656d7f8c368963f2256dc2ef7b7ba89c8
Cr-Commit-Position: refs/heads/master@{#15260}

[charujain, 2016-11-28 13:29:52]

A revert of this CL (patchset #10 id:140001) has been created in
https://codereview.webrtc.org/2534903002/ by charujain@webrtc.org.

The reason for reverting is: Its breaking some trybots due to memory issue. .

[charujain, 2016-11-28 13:33:16]

Patchset #11 (id:150001) has been deleted

[charujain, 2016-11-28 13:33:21]

Patchset #10 (id:140001) has been deleted

[charujain, 2016-11-28 13:33:54]

A revert of this CL (patchset #9 id:130001) has been created in
https://codereview.webrtc.org/2535783002/ by charujain@webrtc.org.

The reason for reverting is: Breaking some trybots due to memory error..

[charujain, 2016-11-28 14:43:32]

Description was changed from

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame.

Also added a unit test case which compares the extracted frame with the frame
stored in text file.

BUG=webrtc:6761

NOPRESUBMIT=true
NOTRY=true

Committed: https://crrev.com/b7636b4656d7f8c368963f2256dc2ef7b7ba89c8
Cr-Commit-Position: refs/heads/master@{#15260}
==========

to

==========
Fixed bug in ExtractFrameFromY4mFile API which was not extracting the frames
correctly.

Issue: This API was calculating the file_header and frame_header offset only for
the first frame which is not the right logic. We need to skip the file and frame
header every time we extract a new frame.

Also added a unit test case which compares the extracted frame with the frame
stored in text file.

BUG=webrtc:6761

NOPRESUBMIT=true

Committed: https://crrev.com/b7636b4656d7f8c368963f2256dc2ef7b7ba89c8
Cr-Commit-Position: refs/heads/master@{#15260}
==========

[charujain, 2016-11-28 14:46:47]

Patchset #10 (id:160001) has been deleted