Build - 2/10/2014 Desktop Nightly STR 1. Go to http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/ 2. Under "Setup Stream for Media Recorder by File", select: ** File: pixel_aspect_ratio.ogg ** Media Type: video ** Mime Type: video/webm 3. Select play on both sets of media controls to start playback of the video 4. Select start recording 5. Wait 10 seconds 6. Select stop recording 7. Select the generated blob URL Expected The video resolution of the original video & the recorded video should be the same. Actual The video resolution of the original video is different the recorded video.

I check the origin file /recording file by vlc. The frame size is the same as 352 x 288. Hi Ralph, Does the testing clips contains possible attributes to make it look widther than normal?

Yes, pixel_aspect_ratio.ogg has a header indicating non-square pixels, as is normal for standard definition video. The intended display size is 525 x 288. The pixel aspect ratio is exaggerated in this file to make square-pixel assumptions obvious in playback. VideoTrackEncoder assumes square pixels, so you need to scale the images before encoding. What happens if you call chunk.mFrame.GetIntrinsicSize() instead of chunk.mFrame.GetImage()->GetSize() in VideoTrackEncoder::NotifyQueuedTrackChanges? That's supposed to be the display size for the frame.

I tried to hard code in WebMElement.c writeVideoTrack Ebml_SerializeUnsigned(glob, DisplayWidth, 525); It works. So, maybe we should pass the display size to webm header instead scale the image?

Can carry display size within metatata to muxer.

Ok, that works too.

1. Change the |Init| function in TrackEncoder 2. For vp8trackEncoder, pass the display width/height to muxer. Hi John & Alfredo: Please help to modify this patch if omx/AVCTrackMetadata needed.

(In reply to Benjamin Chen [:bechen] from comment #8) > Created attachment 8376986 [details] [diff] [review] > bug-970774.patch > > 1. Change the |Init| function in TrackEncoder > 2. For vp8trackEncoder, pass the display width/height to muxer. > > Hi John & Alfredo: > Please help to modify this patch if omx/AVCTrackMetadata needed. There are fields in AVC/H.264 sequence parameter set for 'sample aspect ratio' but they're cannot be set with stagefright encoder implementation. Need to check if decoder handles them or not.

(In reply to John Lin[:jolin][:jhlin] from comment #9) > (In reply to Benjamin Chen [:bechen] from comment #8) > > Created attachment 8376986 [details] [diff] [review] > > bug-970774.patch > > > > 1. Change the |Init| function in TrackEncoder > > 2. For vp8trackEncoder, pass the display width/height to muxer. > > > > Hi John & Alfredo: > > Please help to modify this patch if omx/AVCTrackMetadata needed. > > There are fields in AVC/H.264 sequence parameter set for 'sample aspect > ratio' but they're cannot be set with stagefright encoder implementation. > Need to check if decoder handles them or not. In general, both container and elementary stream both need to have the information the aspect ratio. So decoder decoder can have correct aspect-ratio info no matter it resizes the image on decoding or displaying. Since mp4 container is owned by us, it is possible to do it on container first. Another problem is b2g decoding is replied on android omx currently. Aspect-ratio in meta data [1], so I gues android supports aspect-ratio change on displaying. However, I'm not sure if gecko honours this it or not. [1] http://androidxref.com/4.4.2_r1/xref/frameworks/av/include/media/stagefright/MetaData.h#38

Bruce, I'll handle the mp4 muxing part, could you help to check the gecko decode part?

In MediaOmxReader, only kKeyWidth and kKeyHeight are considered for display: - http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxReader.cpp#144 - http://dxr.mozilla.org/mozilla-central/source/content/media/omx/OmxDecoder.cpp#626 So we might have the same issue on b2g regarding to mp4 video playback. Probably kKeyDisplayWidth and kKeyDisplayHeight should be taken into account as well in this situation. Have we found any mp4 video files that suffer from this issue?

bug 974297 for mp4 muxing part.

Comment on attachment 8376986 [details] [diff] [review] bug-970774.patch Hi Ralph: Would you please help review this patch?

(In reply to Bruce Sun [:brsun] from comment #12) > In MediaOmxReader, only kKeyWidth and kKeyHeight are considered for display: > - > http://dxr.mozilla.org/mozilla-central/source/content/media/omx/ > MediaOmxReader.cpp#144 > - > http://dxr.mozilla.org/mozilla-central/source/content/media/omx/OmxDecoder. > cpp#626 > So we might have the same issue on b2g regarding to mp4 video playback. > > Probably kKeyDisplayWidth and kKeyDisplayHeight should be taken into account > as well in this situation. > > Have we found any mp4 video files that suffer from this issue? http://people.mozilla.org/~ayang/mp4/pixel_aspect_ratio.m4v

Comment on attachment 8376986 [details] [diff] [review] bug-970774.patch Review of attachment 8376986 [details] [diff] [review]: ----------------------------------------------------------------- The fix looks fine to me. Please duplicated in libmkv changes in a patch file checked into the tree and applied by media/libmkv/update.sh so we can maintain the differences against upstream separately. I'd like to see the patch again after that and the nits below are fixed. ::: content/media/encoder/VP8TrackEncoder.cpp @@ +53,5 @@ > } > > nsresult > +VP8TrackEncoder::Init(int32_t aWidth, int32_t aHeight, int aDisplayWidth, > + int aDisplayHeight,TrackRate aTrackRate) These might as well be int32_t as well. @@ +58,2 @@ > { > if (aWidth < 1 || aHeight < 1 || aTrackRate <= 0) { And since they're signed, please check aDisplayWidth/Height are positive here as well. ::: content/media/encoder/VP8TrackEncoder.h @@ +37,5 @@ > nsresult GetEncodedTrack(EncodedFrameContainer& aData) MOZ_FINAL MOZ_OVERRIDE; > > protected: > nsresult Init(int32_t aWidth, int32_t aHeight, > + int aDisplayWidth, int aDisplayHeight, int32_t ::: content/media/webm/EbmlComposer.h @@ +19,5 @@ > /* > * Assign the parameter which header required. > */ > + void SetVideoConfig(uint32_t aWidth, uint32_t aHeight, uint32_t aDisplayWidth, > + uint32_t aDisplayHeight,float aFrameRate); space after comma between arguments.

BTW, ogv and webm both also have a crop rectangle. It's mostly important for frame sizes which aren't a multiple of 8, but we might want to plumb that through in a separate bug.

DisplayWidth/Height elements default to the value of the corresponding PixelWidth/Height elements; it's also worth checking if they match and only writing them if they're different.

1. address review comments 2. new file /media/libmkv/bug970774.patch 3. modify /media/libmkv/update.sh

ni myself

Comment on attachment 8379525 [details] [diff] [review] bug-970774.v01.patch Review of attachment 8379525 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing the update.sh patch date. r=m with the assert nit addressed. ::: content/media/webm/EbmlComposer.cpp @@ +128,4 @@ > float aFrameRate) > { > MOZ_ASSERT(aWidth > 0, "Width should > 0"); > MOZ_ASSERT(aHeight > 0, "Height should > 0"); Asserts for aDisplayWidth/Height as well please. Like PixelWidth/Height these must be positive values by the spec. ::: media/libmkv/WebMElement.c @@ +78,5 @@ > + Ebml_SerializeUnsigned(glob, DisplayWidth, displayWidth); > + } > + if (pixelHeight != displayHeight) { > + Ebml_SerializeUnsigned(glob, DisplayHeight, displayHeight); > + } I would have combined to conditional to write both if ether differs, but ok. We can do our part to test decoder edge cases. :)

Comment on attachment 8379525 [details] [diff] [review] bug-970774.v01.patch Review of attachment 8379525 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OmxTrackEncoder.cpp @@ -30,1 @@ > { Do we need to validate parameter like what you did in VP8TrackEncoder? if (aWidth < 1 || aHeight < 1 || aDisplayWidth < 1 || aDisplayHeight < 1 || aTrackRate <= 0) {

r=rillian try server: https://tbpl.mozilla.org/?tree=Try&rev=ab3eee023129