sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice: > > > > On Jan 20, 2023, at 10:17 AM, Tomas Härdin <g...@haerdin.se> wrote: > > > > ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez: > > > On 18/01/2023 14:40, Tomas Härdin wrote: > > > > Creating a new subthread because I just noticed something > > > > > > I am a bit lost there because the line of code below is not part > > > of > > > this > > > FFV1 patch. > > > Additionally, none on my patches (FFV1 of MXF > > > stored/sampled/displayed > > > fix) modifies the discussed behavior (FFmpeg behavior would be > > > same > > > before and after this patch for MPEG-2 and AVC), so should not > > > block > > > any > > > of them, and a potential fix for that should have its own patch > > > as it > > > would be a separate issue. > > > > True, it doesn't need to hold up this patch. But some discussion is > > warranted I think. I might create a separate patchset for this. > > > > > > > > Anyway: > > > > > > > > > > > > > > > + //Stored height > > > > > mxf_write_local_tag(s, 4, 0x3202); > > > > > avio_wb32(pb, stored_height>>sc->interlaced); > > > > > > > > > Won't this be incorrect for files whose dimensions are > > > > multiples of > > > > 16 > > > > but not multiples of 32? Isn't each field stored separately > > > > with > > > > dimensions a multiple of 16? So while for 1080p we'll have > > > > > > > > StoredHeight = 1088 > > > > SampledHeight = 1080 > > > > > > > > and 1080i: > > > > > > > > StoredHeight = 544 > > > > SampledHeight = 540 > > > > > > > > Where 544 is a multiple of 16, for say 720p we have > > > > > > > > StoredHeight = 720 > > > > SampledHeight = 720 > > > > > > > > but for a hypothetical 720i we'd get > > > > > > > > StoredHeight = 360 > > > > SampledHeight = 360 > > > > > > > > whereas the correct values should be > > > > > > > > StoredHeight = 368 > > > > SampledHeight = 360 > > > > > > AFAIK, it would depend about if the stream has a > > > picture_structure > > > frame > > > (16x16 applies to the frame?) of field (16x16 applies to the > > > field?), > > > but I really don't know enough there for having a relevant > > > opinion. > > > > > > I can just say that I don't change the behavior of FFmpeg in your > > > use > > > case, I found the issues when I tried a random width and height > > > of > > > FFV1 > > > stream then checked with MPEG-2 Video and the sampled width was > > > wrong > > > for sure e.g. sampled width of 1920 for a stream having a width > > > of > > > 1912, > > > with current FFmpeg code, and for your use case I am sure about > > > nothing > > > so I don't change the behavior with my patch, IMO if there is an > > > issue > > > with 720i MPEG-2 Video it should be in a separate topic and patch > > > as > > > it > > > would modify the "stored_height = (st->codecpar- > > > >height+15)/16*16" > > > current code (in my patch I just move this code), unless we are > > > sure > > > of > > > what should be changed on this side and apply a fix on the way. > > > Better > > > to fix 1 issue and let 1 open with no change than fixing no issue > > > because we wouldn't be sure for 1 of the 2. > > > > I suspect we are lucky because 720i doesn't really exist in the > > real > > world, and 576i and 480i are both multiples of 32. > > > > IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height > > are > > "best effort" and thus shall be encoded. Their values will depend > > on > > FrameLayout which in turn depends on what you say - how exactly the > > interlacing is done. > > > > TL;DR: this patchset doesn't need to be held up by this. > > I'm just nudging on the consideration of merging this patch. I've > been testing it over the last week with ffv1/mxf content and have > found this demuxing support very helpful.
Surely you mean muxing? Some FATE tests would be nice. /Tomas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
