mbebenita / Broadway

A JavaScript H.264 decoder.
Other
2.73k stars 424 forks source link

Incorrect frame size from SPS #205

Open Efulefu opened 5 years ago

Efulefu commented 5 years ago

Hello,

I've have a video that, after decoding with Broadway JS, sometimes results in correct frames, sometimes results in a uniformly gray frame.

My process is: Constrained Baseline level 3.0 video encoded with ffmpeg (libx264) v4.1.1, unpacked into a file per Annex-B specs, and streamed through a websocket into a client-side Player object after having split the stream into NALUs.

Here are the command lines used with ffmpeg: h264 encoding: ffmpeg -i <input> -c:v libx264 -vf format=yuv420p -flags +cgop -coder vlc -g 1 -sc_threshold 0 -preset veryslow -crf 15 -movflags +faststart+empty_moov+isml -tune fastdecode -profile:v baseline -level 3.0 <output.mp4> Annex B extraction: ffmpeg -i <output.mp4> -c:v copy -bsf h264_mp4toannexb <output.h264>

Stepping through the code, trying to figure out what was going wrong and where, I've noticed that the height/width deduced by the Player are wrong: the video is 1080x1920, and the Player detects 1088x1920. Most applications I have can't parse the raw Annex B stream, but ffprobe can, and detects the correct dimensions, and both VLC and MediaInfo (and ffprobe) detect the correct dimensions in the .mp4 file.

Here is the SPS packet in hex values, Annex B start code included: 00 00 01 67 42 C0 1E DC 04 40 3C 79 7C 04 40 00 00 03 00 40 00 00 0C 83 C5 8B E0 00

Parsing it by hand I've gotten those values: pic_width_in_mbs_minus1 == 67 (so 68) frame_crop_left_offset == 4 frame_crop_right_offset == 0 68 16 = 1088 SubWidthC == 2 (because 4:2:0) If we take frame_crop_left_offset into account we get the correct value: (1088 - SubWidthC (frame_crop_left_offset + frame_crop_right_offset)) = 1080

It is also my (possibly wrong) understanding that this 8 offset represents a single macroblock (according to an SO reply I stumbled upon, the macroblock width is 16 / SubWidthC ?), and thus means the Decoder is working with the wrong picSizeInMbs and picWidthInMbs values for my video since, after checking the source code, those values seem to be assigned without checking frame_cropping_flag and the associated offsets.

So I have two questions: Can this be the cause of my incorrectly decoded frames? If that is the case, how come some frames come out looking ok (they might have errors I haven't noticed though...) ? I'd be interested in preparing and submitting a pull request to correctly factor in left and right offsets when calculating frame size, however it is my understanding that the Decoder's C code is a fork of an older Android library. Should I still submit a patch to this project, or should I rather look into submitting a PR to the original Android project and then notify you when you can update this project's fork? Also, honestly, if we go forward with this I'll need assistance with testing on all supported videos and formats, so as to make sure I don't introduce a regression just to fix it for my video.

Edit: I think the only place that should require a change would be h264bsdDecodeSeqParamSet() in src/h264bsd_seq_param_set.c, the crop flag and offset values seem to be correctly parsed there, but just aren't calculated back into picWidthInMbs and picHeightInMbs.

P.S. References used for SPS parsing and dimension calculations: ITU H264 specs H264 getting frame height and width from SPS

soliton4 commented 5 years ago

yes thats unfortunately one of the things broadway doesnt handle very well. it all works when you know your video size but when the size is unknown the behavior is somewhat undefined.

here are some hints the video output size in bytes will allways be a multiple of the macro block size in baseline this is 16x16 (if i remember correct)

when you create the canvas and pass the correct video size parameters it will cut away the additional width / height when the video size is unknown this particular part is undefined behavior and could very well be the reason for your gray frames

the good news is that this is actually not very hard to fix. i just need to pass along the parsed video size to the output frames or to some additional event handler. you could either try doing that yourself and make a pr or ask me very very nicely a couple of times. i wanted to do that change anyway and just havent gotten arround to it

Efulefu commented 5 years ago

Oh, so there should be no need to mess with the original H264 C code? I've been trying the fix I suggested but ran into unrelated emscripten issues, so I haven't been able to test the modified macroblock width/height calculations yet.

I'll look into implementing your fix then, definitely seems easier than fixing my emscripten build process for now if it's all in the JS part !

Thanks for the help !

vihangm commented 9 months ago

I am also using broadway player and was very confused why my 640x360 video was rendering as 640x368 with a black bar at the bottom. I too parsed the SPS and realized that the cropping params are in there which then brought me to this open issue. Any chance this can be fixed?

Perhaps if you have pointers for where this needs to be fixed, I can take a stab at it.

vihangm commented 9 months ago

Looks like H264SwDecInfo has the cropping info but the broadwayOnPictureDecoded call uses decInfo.picWidth, decInfo.picHeight for the width and height params instead of cropping them. I guess one could either adjust the size before invoking the callback or include additional cropping params in the callback.