Removes hack that sets the resolution to be a multiple of 8 because the limitation of mjpeg in ffmpeg no longer exists.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- DimesionsLimitation (branched from master)
- Build Status
Buildable 6145 Build 6145: arc lint + arc unit
Event Timeline
Comment Actions
I have been looking at ffmpeg docs, that prescribe this. Blame says nothing, patch works, so I happily hit accept.
@Sergey Sharybin (sergey) Do you know about any problems this could cause?
Comment Actions
This is codec-specific, and not so much a requirement of FFmpeg itself.
Which codec is used here for the proxy output? It may have other requirements for the frame dimensions.
Comment Actions
This is codec specific, but I haven't found any requirements mentioned in ffmpeg docs mentioning this(or anything anywhere).
@Eitan Traurig (EitanSomething) can you link me to something, because I have applied this patch and it works as expected
Comment Actions
JPEG files are in files of 8x8 but if it worked before it might be that ffmpeg handled it by adding a buffer and stores what the actual dimensions are.This was probably needed when ffmpeg didn’t create the buffer but now that it does(I’m assuming this because we had no problems with files that weren’t multiple of 8) we should remove this ‘hack’ because it will probably scale incorrectly with part of the buffer showing because it thinks the video dimensions are bigger that what it actually is.
Where I got the information that it needs to be 8x8
https://dsp.stackexchange.com/questions/35339/jpeg-dct-padding
Comment Actions
I know, that jpeg encoding works on 8x8 blocks, that's why I didn't question these lines too much. If this was ever limitation for MJPEG I would expect at least some historic document describing that.
This is 17x17 pixels MJPEG video created with blender as a "proof"
Comment Actions
I don’t have my computer right now so I can’t test it but if it works for you I have no problem committing it.
Comment Actions
There are still size requirements for various codecs. Here is an example invocation of ffmpeg:
% ffmpeg -i input.mkv -c:v h264 -vf scale=15x17 output-15x17.mkv ffmpeg version 3.4.6-0ubuntu0.18.04.1 Copyright (c) 2000-2019 the FFmpeg developers ... [libx264 @ 0x55573aa25740] width not divisible by 2 (15x17) Error initializing output stream 0:0 -- Error while opening encoder for output stream #0:0 - maybe incorrect parameters such as bit_rate, rate, width or height
I think this should be handled properly before removing that particular code.
Comment Actions
Currently proxy building engine is using MJPEG codec exclusively but you have tested h264.
Can you explain in bit more detail what would be better solution?
Comment Actions
Ah there we go, that was what I was missing. And it's right there under the "Show 20 Lines" button 😴
Comment Actions
PS: a better patch description would have helped tremendously. Especially when it would link to the specification that states the MJPEG requirements for width & height.
Comment Actions
From the code side looks good to me.
But indeed as Sybren mentioned the description (or, commit message) deserves more explanation.
Comment Actions
@Richard Antalik (ISS), don't think @Eitan has commit access. Do you mind making sure this patch gets ocmmitted?
Comment Actions
Yes, will commit.
Sorry, I assumed that this is widely known fact.
I could not find any requirements or limitations for width and height for this codec, will look again and try to quote + add link if I find anything relevant.