Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: "Tomas Härdin" <git@haerdin.se>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support
Date: Tue, 31 Jan 2023 15:53:29 +0100
Message-ID: <09775b73ab48e425f3b557aaa8d5847643ebe9b7.camel@haerdin.se> (raw)
In-Reply-To: <F76F7FE1-61A9-4A17-B0F7-D885F31FEEE5@dericed.com>

sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
> 
> 
> > On Jan 20, 2023, at 10:17 AM, Tomas Härdin <git@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-request@ffmpeg.org with subject "unsubscribe".

  reply	other threads:[~2023-01-31 14:53 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-14 15:45 Jerome Martinez
2023-01-16 14:00 ` Tomas Härdin
2023-01-16 14:17   ` Jerome Martinez
2023-01-18 10:12     ` Tomas Härdin
2023-01-18 14:25       ` Jerome Martinez
2023-01-18 13:40 ` Tomas Härdin
2023-01-18 14:15   ` Jerome Martinez
2023-01-20 15:17     ` Tomas Härdin
2023-01-29 16:36       ` Dave Rice
2023-01-31 14:53         ` Tomas Härdin [this message]
2023-03-14  9:52           ` Jerome Martinez
2023-03-15 14:00             ` Tomas Härdin
2023-03-24 13:59               ` Dave Rice
2023-03-25 18:29                 ` Tomas Härdin
2023-04-01 14:37             ` Michael Niedermayer
2023-04-01 15:20               ` Jerome Martinez
2023-04-01 15:43                 ` Michael Niedermayer
2023-04-01 17:11                   ` Jerome Martinez
2023-04-02 20:07                     ` Michael Niedermayer
2023-04-02 22:07                       ` Jerome Martinez
2023-04-04 14:43                         ` Michael Niedermayer
2023-04-04 14:57                           ` Jerome Martinez
2023-04-04 17:32                             ` Michael Niedermayer
2023-04-04 21:37                               ` Jerome Martinez
2023-04-05 12:31                                 ` Michael Niedermayer
2023-04-05 12:42                                   ` Michael Niedermayer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=09775b73ab48e425f3b557aaa8d5847643ebe9b7.camel@haerdin.se \
    --to=git@haerdin.se \
    --cc=ffmpeg-devel@ffmpeg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git