Update padding/alignment constants to match ffmpeg.

Update padding/alignment constants to match ffmpeg.

Our alignment is out of sync with ffmpeg for AVX architectures. This was
causing crashes in the ffmepg vp9 decoder (not yet shipped).

BUG=481396, 150920

[chcunningham, 2015-08-10 23:44:09]

chcunningham@chromium.org changed reviewers:
+ dalecurtis@chromium.org, xhwang@chromium.org

[chcunningham, 2015-08-10 23:44:09]

This change is perhaps not finished, though it does eliminate the crashes I was
seeing in ffvp9. (Crashes observed using both MSE and SRC= to play "feelings"
vp9 video at http://shaka-player-demo.appspot.com)

xhwang made relevant changes before
 DecoderBuffer - https://chromiumcodereview.appspot.com/10949029/
 VideoFrame - https://chromiumcodereview.appspot.com/11308310


My changes so far only involve VideoFrame. I'm less familiar with the origins of
the paddinging/alignment constants used DecoderBuffer, but perhaps they too need
to be updated. My guess is we should mirror the allocation of AVPacket?


For kAlignmentSize, my guess is the ffmpeg equivalent is here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

Questions about kAlignmentSize:
1. av_malloc (using this alignment) is called in paths of allocating AVFrame
(chromium analog: VideoFrame) as well as AVPacket (chromium: DecoderBuffer).
Should we just define this alignment constant in one place and it in both
VideoFrame & DecoderBuffer? 
2. Even if defining it in two places, I think VideoFrame should be updated from
16 -> 32 for non-arm (DecoderBuffer already does this). WDYT?


Looking at av_new_packet
(http://ffmpeg.org/doxygen/trunk/avpacket_8c_source.html#l00083)

I see they always use a padding size of 32
(http://ffmpeg.org/doxygen/trunk/libavcodec_2avcodec_8h_source.html#l00635). We
are currently using 16 in DecoderBuffer. Update this as well?

[chcunningham, 2015-08-10 23:45:36]

https://codereview.chromium.org/1286623002/diff/1/media/base/video_frame_unit...
File media/base/video_frame_unittest.cc (left):

https://codereview.chromium.org/1286623002/diff/1/media/base/video_frame_unit...
media/base/video_frame_unittest.cc:52: yv12_frame->coded_size().width() &
(VideoFrame::kFrameSizeAlignment - 1),
I don't think this was asserting the correct thing before (and happened to pass
because of width/height values chosen). The assert fails with my latest changes
and passes after I update it to assert on stride size instead of coded_size

[chcunningham, 2015-08-11 00:16:05]

BTW: pre-req ffmpeg changes here 
https://chromium-review.googlesource.com/#/c/292482/1

[DaleCurtis, 2015-08-11 00:31:29]

dalecurtis@chromium.org changed reviewers:
+ miu@chromium.org

[DaleCurtis, 2015-08-11 00:31:30]

+miu for an insight into my ffmpeg zaniness :)

https://codereview.chromium.org/1286623002/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1286623002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:31: // These values are chosen according to like values
in FFmpeg. See static
Hmm, I wonder if we should instead add a constructor so that ffmpeg frames can
specify the right stride and such, but normal frames don't waste as much memory.

[DaleCurtis, 2015-08-11 00:31:47]

s/my/more/ :)

[miu, 2015-08-11 02:53:55]

https://codereview.chromium.org/1286623002/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1286623002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:31: // These values are chosen according to like values
in FFmpeg. See static
On 2015/08/11 00:31:30, DaleCurtis wrote:
> Hmm, I wonder if we should instead add a constructor so that ffmpeg frames can
> specify the right stride and such, but normal frames don't waste as much
memory.

+1.  This approach wastes memory for the non-ffmpeg producers of VideoFrames. 
Actually, the existing approach (16-byte alignment) also wastes memory.  But,
this CL proposes to waste more.  ;-)

Can FFMPEG specify the alignment- and padding-adjusted coded size using this
ctor:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...

Also, it feels like we've made a huge mistake [in the past], in that
VideoFrame::CreateFrame() can create a frame with a larger coded size than what
was provided.  There are plenty of use cases (e.g., HW codecs) where an EXACT
coded size is required.  FWICT, it just so happens that 16-byte alignment works
for everything right now.

[wtc1, 2015-09-05 01:46:04]

wtc@google.com changed reviewers:
+ wtc@google.com

[wtc1, 2015-09-05 01:46:05]

https://codereview.chromium.org/1286623002/diff/1/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

https://codereview.chromium.org/1286623002/diff/1/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.h:34: #include <libavcodec/internal.h>

The "internal.h" file name suggests this header is internal. Also it is not
installed in /usr/include/libavcodec on my Ubuntu 14.04 computer.

[miu, 2015-09-29 18:47:20]

Is this issue still alive?  Should we close it, or should I proceed with another
round of code review?

[chcunningham, 2015-09-30 18:22:02]

Lets close it. The refactoring we discussed is better and this fix isn't
un-blocking anyone AFAIK.